Skip to main content
Commonmark migration
Source Link

#SinglyLinkedList

SinglyLinkedList

#LinkedListTester

LinkedListTester

#LinkedListerTraverser

LinkedListerTraverser

#SinglyLinkedList

#LinkedListTester

#LinkedListerTraverser

SinglyLinkedList

LinkedListTester

LinkedListerTraverser

Source Link
maaartinus
  • 13.5k
  • 1
  • 34
  • 73

While reinventing-the-wheel is often fun, you needn't go that far when exercising. Even when writing your own SinglyLinkedList, you should try to fit it to the existing interface List. This way you both exercise programming and learn about existing classes.

There's no equals and no hashCode. You really should write them.

#SinglyLinkedList

  ILinkedListNode<T>       root      = null;
  ILinkedListNode<T>       last      = null;
  int                      size      = 0;

This looks more like a fill-in form than like a Java code. If you want a table, use a spreadsheet.

  • It adds no value to your code and just costs time.
  • It also breaks after renaming.
  • Moreover, it makes problem together with version control (e.g., git) as it may force you to introduce needless changes (just imagine you use some reallyLongNameNotFittingInYour column.

Explicitly initializing fields to their default values buys you nothing. Just leave it out, there's no risk involved in

  ILinkedListNode<T> root;
  ILinkedListNode<T> last;
  int size;

This makes it also easier to spat the places where you do some non-default initialization.


  @Override
  public ILinkedListNode<T> getRoot() {
    return root;
  }

Look at java.util.LinkedList. It has a Node, but keeps it private. Always keep everything as much private as possible.


  LinkedListerTraverser<T> traverser = new LinkedListerTraverser<T>(root, size);

  @Override
  public LinkedListerTraverser<T> getTraverser() {
    traverser.setRoot(root, size);
    return traverser;
  }

This is probably wrong. You're reusing a traverser, so that there can be just one per list. Something like

for (String s1 : list) {
    for (String s2 : list) {
        ...
    }
}

wouldn't work with your list, even when you turned your traverser into an Iterator.


  throw new RuntimeException("Negative Index not accepted.");

Should be IllegalArgumentException or better IndexOutOfBoundsException.

#LinkedListTester

public class LinkedListTester {

Use JUnit. It's not hard to learn and everybody uses it (or some better but similar alternative).

System.out.println("Test Case 1: Appending 1 element.");

You really don't want to print anything when you run thousands of tests.

ILinkedList<Integer> LT = new SinglyLinkedList<Integer>();

LT is a wrong name, it should start lowercase and why "T"? Just call it List.


Rewriting you whole test with JUnit should take 10 minutes at most. This is the start

public class LinkedListTest extends TestCase {
    public void testAppend_one_element() {
        ILinkedList<Integer> list = new SinglyLinkedList<Integer>();
        list.append(1);
        list.getTraverser().traverseList();
        assertEquals(1, list.size());
    }

    ...
}

As you can see, naming methods in tests is not that strict. It's usually test + capitalized method name and you may add an underscore and some details. Some people call their methods like test_that_adding_two_and_three_gives_five.

No idea what the traverser does in the above test. If you implemented List and used Guava, you could simplify your test to

    public void test_appending_one_element() {
        ILinkedList<Integer> list = new SinglyLinkedList<Integer>();
        list.append(1);
        assertEquals(ImmutableList.of(1), list);
    }

which is more descriptive.

#LinkedListerTraverser

It looks like all it does is printing. Implement toString in your list (you may need a helper class for this, but probably not).