current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 5 down vote favorite
1

I am curious as to what improvements to my code can be suggested.

package concepts;

import java.util.Iterator;
import java.util.NoSuchElementException;

public class LinkedList<M> implements Iterable<LinkedListNode<M>> {

    public static class LinkedListIterator<O> implements Iterator<LinkedListNode<O>> {
        private LinkedListNode<O> curNode;

        public LinkedListIterator(LinkedList<O> linkedList) {
            curNode = linkedList.head;
        }

        @Override
        public boolean hasNext() {
            if (curNode != null) {
                return true;
            } else {
                return false;
            }
        }

        @Override
        public LinkedListNode<O> next() {
            if (!hasNext()) {
                throw new NoSuchElementException();
            } else {
                LinkedListNode<O> temp = curNode;
                curNode = curNode.next;
                return temp;
            }

        }

    }

    public LinkedListNode<M> head;

    public boolean add(M newData) {
        LinkedListNode<M> newNode = new LinkedListNode<M>();
        newNode.data = newData;
        if (head == null) {
            head = newNode;
        } else {
            LinkedListIterator<M> it = new LinkedListIterator<M>(this);
            LinkedListNode<M> lastNode = null;
            while (it.hasNext()) {
                lastNode = it.next();
            }
            if (lastNode != null) {
                lastNode.next = newNode;
            } else {
                throw new IllegalStateException();
            }
        }
        return true;
    }

    @Override
    public Iterator<LinkedListNode<M>> iterator() {
        return new LinkedListIterator<M>(this);
    }

}

Here is the LinkedListNode implementation:

package concepts;

public class LinkedListNode<N> {
    public N data;
    public LinkedListNode<N> next;
}
share|improve this question
up vote 6 down vote

Antipattern

It's not a good idea to expose the internal list nodes, and this is why:

public static void main(String[] args) {
    LinkedList<Integer> list = new LinkedList<>();
    list.add(123);
    // Make a loop:
    list.head.next = list.head;
    int i = 0;

    for (LinkedListNode<Integer> node : list) {
        ++i;

        if (i == 100) {
            break;
        }
    }

    System.out.println("i = " + i);
}

As the above snippet demonstrates, a user may restructure your linked list into a cycle in which iteration will never terminate. So the moral here is that:

hide implementation from the user!

First of all, make head a private field. Then, reimplement the iterator that iterates over values and not the internal nodes holding those values.

Additional field tail would not hurt, while keeping your add method a constant time operation.

Summa summarum

All in all, I had this in mind:

package concepts;

import java.util.Iterator;
import java.util.NoSuchElementException;

public class LinkedListCR<E> implements Iterable<E> {

    private static final class LinkedListNode<E> {
        private E element;
        private LinkedListNode<E> next;

        LinkedListNode(E element) {
            this.element = element;
        }
    }

    private class LinkedListIterator implements Iterator<E> {

        private LinkedListNode<E> nextNodeToReturn = head;

        @Override
        public boolean hasNext() {
            return nextNodeToReturn != null;
        }

        @Override
        public E next() {
            if (!hasNext()) {
                throw new NoSuchElementException("Iterator exceeded.");
            }

            E ret = nextNodeToReturn.element;
            nextNodeToReturn = nextNodeToReturn.next;
            return ret;
        }

    }

    private LinkedListNode<E> head;
    private LinkedListNode<E> tail;

    public void add(E element) {
        LinkedListNode<E> newnode = new LinkedListNode<>(element);

        if (head == null) {
            head = newnode;
            tail = newnode;
        } else {
            tail.next = newnode;
            tail = newnode;
        }
    }

    @Override
    public Iterator<E> iterator() {
        return new LinkedListIterator();
    }

    public static void main(String[] args) {
        LinkedListCR<String> list = new LinkedListCR<>();
        list.add("hello");
        list.add("world");
        list.add("y'all");

        for (String s : list) {
            System.out.print(s + " ");
        }

        System.out.println();
    }
}

Hope that helps.

share|improve this answer
1  
Thanks for the suggestions! – Dhruva Bharadwaj Sep 18 '16 at 9:52

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.