Using JUnit to test a method that reverses a singly linked list

Question

I'm implementing a singly linked list in Java. I've created a method called reverseList which reverses my list, for example:

head -> 1 -> 2 -> 3 -> null
Becomes:
head -> 3 -> 2 -> 1 -> null

I've also created a test class (using JUnit) that has a method called testReverseList which tests the reverseList method. The insert method takes the data and also the position in which you want to add a new node. All these three methods seems to be working correctly.

public String reverseList() {
    Node prev = null, current = head, next;
    StringBuilder checkList = new StringBuilder();
    while (current != null) {
        checkList.append(current.getData());
        next = current.getNext();
        current.setNext(prev);
        prev = current; 
        current = next;
    }
    head = prev;
    return checkList.toString(); 
}

@Test
public void testReverseList() {
    LinkedList myList = new LinkedList();
    myList.insert(1, 1);
    myList.insert(2, 2);
    myList.insert(3, 3);
    String normalListOrder = "123";
    assertEquals("Check reverseList, reversing a list with 3 elements.", normalListOrder, myList.reverseList());    
}

My question is: I don't believe that I'm properly testing my reverseList method by using this approach of appending each data of my node and then returning it, because I'm not really checking if it was reversed or not. Furthermore, it looks like it would require more computational resources to do all this. And I don't think that just returning a true at the end of execution would be enough either. So, should I don't test it at all? Is there a better way to do the testing?

ANSWER: Thanks everyone. I've followed Pelocho suggestion and overridden the equals method and changed my testReverseList and reverseList methods. I also created testEquals method to check the implemented equals. It seems that this is the best approach for what I want. All the tests are OK.

public LinkedList reverseList() {
    Node prev = null, current = head, next;
    while (current != null) {
        next = current.getNext();
        current.setNext(prev);
        prev = current;
        current = next;
    }
    head = prev;
    return this;
}

public boolean equals(LinkedList myList) {
    if (myList == this) {
        return true;
    }
    if (!(myList.getListSize() == this.getListSize())) {
        return false;
    }
    // Starts at 1 because retrieveNode pick the position, not index
    for (int i = 1; i != this.getListSize(); i++) {
        if (myList.retrieveNode(i).getData() != this.retrieveNode(i).getData()) {
            return false;
        }
    }
    return true;
}

@Test
public void testReverseList() {
    LinkedList myList = new LinkedList();
    myList.insert(1, 1);
    LinkedList singleElementList = new LinkedList();
    singleElementList.insert(1, 1);
    assertEquals("Check reverseList, reversing a list with 1 element.", true, myList.reverseList().equals(singleElementList));  
    LinkedList expectedList = new LinkedList();
    myList.insert(2, 2);
    myList.insert(3, 3);
    expectedList.insert(3, 1);
    expectedList.insert(2, 2);
    expectedList.insert(1, 3);
    assertEquals("Check reverseList, reversing a list with 3 elements.", true, myList.reverseList().equals(expectedList));  
}

public void testEquals() {
    LinkedList myList = new LinkedList();
    myList.insert(1, 1);
    assertEquals("Check reverseList, comparing to the same list.", true, myList.reverseList().equals(myList));
    LinkedList myList2 = new LinkedList();
    myList2.insert(1, 1);
    myList2.insert(2, 2);
    assertEquals("Check reverseList, different size list.", false, myList.reverseList().equals(myList2));
}

Answer 1

I'd start suggesting to return a new LinkedList when reversing it. But that's just for the sake of immutability

Anyway, you want to test your method, right?

Right now you're altering the state and returning a representation of current state just for the sake of testing. Be aware that modifying your production code just to be able to perform tests is not usually a good practice. You're modifying its inner state so that's what you need to check

@Test
public void testReverseList() {
    LinkedList someList = new LinkedList();
    myList.insert(1, 1);
    myList.insert(2, 2);
    myList.insert(3, 3);

    LinkedList expectedList = new LinkedList();
    // You'll probably want to check these lines in order to ensure I'm using it the right way
    expectedList.insert(3, 1);
    expectedList.insert(2, 2);
    expectedList.insert(1, 3);

    for (int i = 0; i < expectedList.size(); i++)
        assertEquals("Checking item with index " + i, myList.get(i), expectedList.get(i));
    assertEquals(myList.size(), expectedList.size());

    // Or even better, implement LinkedList.equals() so you can use
    assertEquals("Check reverseList", myList, expectedList);    
}

By performing the check using toString() you're relaying too much in its implementation. Let's say you have an Object whose toString() method returns a random String (I'm not sure why that could be useful but let's assume that for a minute). assertEquals(myObject.toString(), myObject.toString()) would probably fail but assertEquals(myObject, myObject) would not

Answer 2

May I suggest the following changes:

Separate the list reversal from the "toString" representation, and call the methods separately.

Then, make sure you are testing for the correct result (you want "321", right?).

public LinkedList reverseList() {
    Node prev = null, current = head, next;
    while (current != null) {
        next = current.getNext();
        current.setNext(prev);
        prev = current; 
        current = next;
    }
    head = current;
    return this;
}

public String toString() {
    StringBuilder checkList = new StringBuilder();
    Node current = head;
    while (current != null) {
        checkList.append(current.getData());
        current = current.getNext();
    }
    return checkList.toString(); 
}

@Test
public void testReverseList() {
    LinkedList myList = new LinkedList();
    myList.insert(1, 1);
    myList.insert(2, 2);
    myList.insert(3, 3);
    String reverseListOrder = "321";
    assertEquals("Check reverseList, reversing a list with 3 elements.", reverseListOrder, myList.reverseList().toString());    
}

Answer 3

It is kind of hard to understand your code. If you need a code review I can begin by telling you never use the same class names as the ones in the API(LinkedList). Then make a separate class and use junit for the tests. Your test fails ? You are comparing "123" with what should be "321" no ? And why does your list has a method that inserts a key value pair ?