3
\$\begingroup\$

Please review my code.

public class CircularLinkedlistinsert {
    private Node first;

    private static class Node {
        int element;
        Node next;

        public Node(int element, Node next) {
            this.element = element;
            this.next = next;
        }
    }

    public void insertNodeInCircularLL(int element) {
        Node node = new Node(element, null);
        if (first == null) {
            first = node;
            first.next = first;
        } else {
            Node current = first;
            do {
                if (((current.element <= element)  && (current.next.element >= element)) || current.next == first)  {
                    node.next = current.next;
                    current.next = node;
                    if (element < first.element) {
                        first = node;
                    }
                    return;
                }
                current = current.next;
            } while (current != first);
        }
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ You've fallen into the indentation trap. Add a return statement to the true case of the if statement and get ride of the else syntax. \$\endgroup\$ – ChaosPandion Aug 26 '13 at 3:36
4
\$\begingroup\$
public class CircularLinkedlistinsert {

Class names should have the first letter of each word capitalized. I'd also change the name to remove the activity and just describe the object. i.e., CircularLinkedList. The method names will describe whatever action is taken.

private static class Node {
    int element;
    Node next;

While it doesn't matter too much since this is a private nested class, the element and next fields should in theory be private as well and accessed via getElement() / getNext() and setElement() / setNext() methods. This promotes encapsulation and good object-oriented practices. (This is a purist observation, and what you've done works fine for your model.)

public void insertNodeInCircularLL(int element)

Again, I'd rename this method for style points. Since the entire class is a CircularLinkedList, we can just call this insertNode(), or even something as simple as add() or put().

The problem with the rest of the implementation, to me, is that you are using the element field both as the content of the Node and its position in the linked list. That's a problem. What if I pass in an int to the method that isn't the next logical number in the sequence? What if I pass in something like -1?

It looks like, based on this if statement (((current.element <= element) && (current.next.element >= element))), that you're trying to facilitate inserting a node in between two other nodes. I think there might be a more sane way to do it, however even if you want to continue with this approach and allow the client code to pass in an index directly, you will need to do some sanity checking of the input before processing.

I think you need to have the element field be something managed internally, with only the content of the node passed in to the insert method. This is more of a logic error in your implementation than an issue with your coding style or practices, though.

Two more advanced notes from this:

  • If you do end up generating the IDs yourself, you ought to look into synchronization to avoid problems with multithreading.

  • This CircularLinkedList can only accept nodes which hold int information. You could change the element field to be a true id, and then use Generics to make your implementation truly powerful.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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