There are several things to take into account in your code. I don’t know if your teacher is a good boy or an executioner or how much he forgives or punishes the student for silly details. So I’ll tell you anything you think is wrong.
When instantiating generic classes, the compiler is (from Java 7) able to in many cases guess the generic type of the constructor, which can be omitted. So you can change that:
LinearNode < T > newNode = new LinearNode < T > (element);
That’s why:
LinearNode <T> newNode = new LinearNode<>(element);
Note the <>
in the constructor. This is called the diamond syntax. (reference).
Never put a System.out.println
within its method toString()
. In fact, never do it on your toString()
anything that could cause a side effect. One of the reasons is that Netbeans in debug mode calls the toString()
on objects you are inspecting in the debug. However, if the method toString()
can cause side effects (even a simple System.out.println
), the output of your program will be very confusing and will not correspond to the same output that it would produce in production environment. Moreover, it is very easy to make the method toString()
be implicitly called by Java in several places.
In addition, the purpose of the method toString()
is to provide a representation in the format of String
object. This means writing anything on the console is not part of this method. Therefore, a logic that writes something in the console within the toString()
should be seen as an undesirable and harmful intruder to be removed from the code.
I also strongly suspect that your method contains
follows a similar logic and the System.out.println
inside it is harmful.
Let’s see your method indexOf
:
if (current.equals(obj)) {
Well, obj
is the element you want to find in the list, but current
is a list node, not an element. Therefore, this equals
will never be true and the result will always be -1. What you wanted is this:
if (current.getElement().equals(obj)) {
However, if the list item is null
, this will give NullPointerException
. Therefore, the solution is to use the method Objects.equals(Object, Object)
:
public int indexOf(Object obj) {
int index = 0;
LinearNode<T> current = head;
while (current != null) {
if (Objects.equals(current.getElement(), obj)) return index;
index++;
current = current.getNext();
}
return -1;
}
If you prefer, you can use a for
:
public int indexOf(Object obj) {
int index = 0;
for (LinearNode<T> current = head; current != null; current = current.getNext()) {
if (Objects.equals(current.getElement(), obj)) return index;
index++;
}
return -1;
}
Your method contains
can be simplified by using the indexOf
thus:
public boolean contains(T targetElement) {
return indexOf(targetElement) != -1;
}
However, if you don’t want to use the indexOf
within the contains
, we can see that the variable found
is not necessary if you give return true;
instead of found = true;
, leaving a return false;
at the end of the method. The variable result
also not required. The code would look like this:
public boolean contains(T targetElement) {
LinearNode<T> current = head;
while (current != null) {
if (Objects.equals(targetElement, current.getElement())) return true;
current = current.getNext();
}
return false;
}
Note that when the condition of if
is true, the flow has no way to proceed in the following instruction to the body of if
(the current = current.getNext();
) because of the return
. Therefore, it is not necessary to place the following content to the body of the if
within a block else
.
You can also choose to use a for
:
public boolean contains(T targetElement) {
for (LinearNode<T> current = head; current != null; current = current.getNext()) {
if (Objects.equals(targetElement, current.getElement())) return true;
}
return false;
}
Avoid concatenating many String
temporary s (their method toString()
does so). Each time you concatenate a String
, a new String
is created in memory containing copies of each concatenated piece, rather than changing existing pieces. This is inefficient as several String
are created, copied up and down and dropped into memory to be collected as garbage. It is more efficient to use the StringBuilder
which is an object that undergoes changes in concatenation, rather than being created a new object.
Here’s how your revised code looks:
@Override
public String toString() {
LinearNode<T> current = head;
StringBuilder result = new StringBuilder();
while (current != null) {
result.append(current.getElement()).append('\n');
current = current.getNext();
}
return result.toString();
}
If you prefer, you can use a for
:
@Override
public String toString() {
StringBuilder result = new StringBuilder();
for (LinearNode<T> current = head; current != null; current = current.getNext()) {
result.append(current.getElement()).append('\n');
}
return result.toString();
}
Finally, based on the version I did above the method contains
without using the indexOf
and using the for
, we can create a method countOccurrences
:
public int countOccurrences(T targetElement) {
int found = 0;
for (LinearNode<T> current = head; current != null; current = current.getNext()) {
if (Objects.equals(targetElement, current.getElement())) found++;
}
return found;
}
Let’s see your method add
:
if (head == null) {
head = newNode;
newNode.setNext(tail);
If head
is null
, tail
will also be null
, and therefore, you are doing newNode.setNext(null);
, which won’t make a difference since the newNode
is newly created and the next
of his already gone null
.
head = oldHead;
At this point, oldHead
is already a reference to head
. So in practice this will do nothing.
oldHead.setNext(newNode);
}
You set that old head that pointed to an item x
, now points to a new item newNode
. However, this does not work and causes the sublist of the item x
forward is lost, besides you do not change the head of the list. You also never define the tail
in the method add
. Therefore, your list will never have more than two elements.
Here is a way to correct the method add
. Since I don’t know if the idea was to add at the beginning or at the end, I put both:
public void addFirst(T element) {
LinearNode<T> newNode = new LinearNode<>(element);
newNode.setNext(head);
head = newNode;
if (tail == null) tail = newNode;
count++;
}
public void addLast(T element) {
LinearNode<T> newNode = new LinearNode<>(element);
if (tail == null) {
head = newNode;
} else {
tail.setNext(newNode);
}
tail = newNode;
count++;
}
Let’s see your method remove
. First that she has a kind of return that is the following:
return current.getElement();
Now, if the element has been found, that’s the same as:
return element;
Already in case the element is not on the list, will give a NullPointerException
in the } else if (current.equals(head)) {
. Use the Objects.equals(Object, Object)
will arrange the else if
s, but will make the NullPointerException
go into the else
. This makes me wonder what is the usefulness of the return of this method, since it is always the parameter itself (if no exception occurs). Therefore, I think it would be better if the return were boolean
, where true
indicates that the element has been found and removed, and false
indicates otherwise.
We can start fiddling with the code by improving the logic of while
which finds the desired element. In this while
, we can eliminate the variable found
when using the break
thus:
while (current != null) {
if (element.equals(current.getElement())) break;
previous = current;
current = current.getNext();
}
if (current == null) return false;
Note that in this case, if it iterates the while
until the end and not find the element, the if
soon after the method ends without anything being done returning false
, and this also occurs if the list is empty. Since the if
having the true condition executes the break
and never continues the flow normally after the if
, then the else
is not necessary.
Further down, in the sequence of if
s and else if
s, the code is correct (now that we’ve secured that the item has been found). However, you can improve it by using the ==
instead of equals
, once you are going through the list’s own nodes, and never copies or clones of those nodes. At the end, you give a return true;
. Finally, the System.out.println
s are intruders to be removed from the code.
That’s your revised method:
public boolean remove(T element) {
LinearNode<T> previous = null;
LinearNode<T> current = head;
while (current != null) {
if (element.equals(current.getElement())) break;
previous = current;
current = current.getNext();
}
if (current == null) return false;
if (this.count == 1) {
head = tail = null;
} else if (current == head) {
head = current.getNext();
} else if (current == tail) {
tail = previous;
tail.setNext(null);
} else {
previous.setNext(current.getNext());
}
count--;
return true;
}
What is the class code
LinearNode
?– Victor Stafusa