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 Stringtemporary 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 Stringare 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 ifs, 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 ifs and else ifs, 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.printlns 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