Remove item after a specified item from a linked list

Question

I have written a function that deletes the item that is next to the one being passed as a parameter. So far this function deletes but I feel like there is a better way to implement this.

public class MyLinked{
private class Node{
 Node next;
 double item;
 public Node(double item, Node next) {
 this.item = item;
 this.next = next;
 }
 public Node() {
 }
}
Node first;

Here is the function:

public void removeAfter(double x) {
 if(first == null) { return;}
 else {
 Node c = first;
 while(c!=null){
 if(c.item == x && c.next!=null) {
 c.next = c.next.next;
 }
 c = c.next;
 }
 }
}

Answer 1

 if(first == null) { return;}
 else {
 Node c = first;
 while(c!=null){
 if(c.item == x && c.next!=null) {
 c.next = c.next.next;
 }
 c = c.next;
 }
 }

This could be written more briefly as

 for (Node c = first; c != null; c = c.next) {
 if (c.item == x && c.next != null) {
 c.next = c.next.next;
 return;
 }
 }

You don't need the first if, as the code has the same effect without it.

A for loop better captures the iterative properties here.

If we return immediately after doing our task, we save some iterations.

Another alternative would be

 if (first == null) {
 return;
 }
 for (Node current = first; current.next != null; current = current.next) {
 if (current.item == needle) {
 current.next = current.next.next;
 return;
 }
 }

I would prefer not to put everything on the same line. This would be even more true if there was an else.

Since we return, we don't need an else. This has the same effect without it, and we save the extra indentation.

I find current more descriptive than c. Not a huge deal for a looping variable.

Since we know that first is not null, we can go ahead and check the next. This saves us an iteration if we're looking for the last item. This also saves having to check next later.

I find needle (meaning something for which we're searching) more descriptive than x. That's a common idiom, but you can say target, etc. if that doesn't work for you.

You might consider returning the value that you remove.

 double value = current.next.item;
 current.next = current.next.next;
 return value;

This is common with remove operations. You have the value right there at the time. And this saves having to traverse the list twice to get the item and then remove it. Of course, perhaps your usage patterns never have you doing that.

• EastXWest: @J.user94 nicely put
• Stingy: By adding a return statement inside the if clause in the loop, you are altering the method's behavior, because the needle might appear more than once in the list. The original method of @J.user94 would delete all items in the list that are preceded by the needle, whereas yours only removes the first such item.

Answer 2

a better way to implement this

Your way looks fine. I'd say keep the code in the while loop.

The initial if is a bit odd, as it is redundant with the loop's logic. Just delete that initial test.

Perhaps that bitwise equality test on the double is exactly what you want, but many folks will choose to write x == y as Math.abs(x - y) < epsilon. You might prefer to have removeAfter accept a Node parameter, and to push the while loop into a separate findNode method.