current community

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite 
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);
    }
    }
}
}
share|improve this question
2  
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. –  ChaosPandion Aug 26 '13 at 3:36
add comment

1 Answer

up vote 2 down vote accepted 
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.

share|improve this answer
add comment

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.