Optimizing squish() method further

Part II of this question says:


Fill in the squish() method in the SList class so that it performs as indicated in the comment. Your solution should not use arrays, nor should it use your smoosh() method. Do not change the prototype of the SList constructor or the insertEnd method; our test software will call them.

Below is the code written as a solution in squish() method. Modular testing is done for this method.

```
public class SList {

private SListNode head;
private int size;

/**
* SList() constructs an empty list.
**/

public SList() {
size = 0;
head = null;
}

/**
* isEmpty() indicates whether the list is empty.
* @return true if the list is empty, false otherwise.
**/

public boolean isEmpty() {
return size == 0;
}

/**
* length() returns the length of this list.
* @return the length of this list.
**/

public int length() {
return size;
}

/**
* insertFront() inserts item "obj" at the beginning of this list.
* @param obj the item to be inserted.
**/

public void insertFront(Object obj) {
head = new SListNode(obj, head);
size++;
}

/**
* insertEnd() inserts item "obj" at the end of this list.
* @param obj the item to be inserted.
**/

public void insertEnd(Object obj) {
if (head == null) {
head = new SListNode(obj);
} else {
SListNode node = head;
while (node.next != null) {
node = node.next;
}
node.next = new SListNode(obj);
}
size++;
}

/**
* squish() takes this list and, wherever two or more consecutive items are
* equal(), it removes duplicate nodes so that only one consecutive copy
* remains. Hence, no two consecutive items in this list are equal() upon
* completion of the procedure.
*
* After squish() executes, the list may well be shorter than when squish()
* began. No extra items are added to make up for those removed.
*
* For example,

I wouldn't worry too much about line numbers. The way I take it, the task doesn't say that you have to stay below 11 lines, but just that their solution is 11 lines, so that you have a general idea how long a solution might be (so if your at e.g. 50 lines, you know you are off track).

size

Your local size variable is poorly named, as it doesn't represent the size of the list, but is just an iterator variable.

Also, the size field will contain the wrong value if squish removed a node.

Early Return and simplified size check

You can combine your size checks:

if (size < 2) {
        return;
    }

And then you can assign prevNode and curNode like this:

SListNode prevNode = this.head;
    SListNode curNode = this.head.next;

This is more readable and saves you lines.

Null Values and toString

The value of a node can be null right now as insert doesn't check for this. But your squish method cannot handle null values.

equals and toString

Remove the call to toString, it doesn't make that much sense, and might return false results, depending on the equals implementation.

Checking if next node is null

You are handling this inconsistently: The first time, you use if (size >= 2) to check if curNode.next will be null, the second time you use if (curNode.next != null). You should use the same check in both cases.

Also you actually only need to check once: You can move it outside the if statement as it gets executed either way.

And I don't think that you even need that check. If size is smaller than 2, the while loop will stop anyways.

Code

If you follow all the advice, your code would look like this:

// NOTE: size field might be wrong after calling this method, and it doesn't work with null values in nodes
public void squish() {
    // Fill in your solution here. (Ours is eleven lines long.)
    if (size = 2) {
        if (prevNode.item.equals(curNode.item)) {
            prevNode.next = curNode.next;
        } else {
            prevNode = curNode;
        }
        curNode = curNode.next;
        size--;
    }
}

Which is already a bit shorter. I wouldn't reduce the lines any further, it will just decrease readability.

But if you really want to, you could remove curNode, and thus also the check:

public void squish() {
    SListNode prevNode = this.head;
    while (size >= 2) {
        if (prevNode.item.equals(prevNode.next.item)) {
            prevNode.next = prevNode.next.next;
        } else {
            prevNode = prevNode.next;
        }
        size--;
    }
}

This would also give you two lines to spare, which is even enough to properly maintain the size field:

// maintains size field, but still doesn't handle null values in nodes
public void squish() {
    SListNode prevNode = this.head;
    for (int i = this.size; i >=2; i--) {
        if (prevNode.item.equals(prevNode.next.item)) {
            prevNode.next = prevNode.next.next;
            this.size--;
        } else {
            prevNode = prevNode.next;
        }
    }
}

StackExchange Code Review Q#67997, answer score: 3