5
\$\begingroup\$

I'm looking for a review paying special attention as to whether the code can be optimized in the form of number of variables used, especially for the delete() method.

public class circularLL {

    Node start;
    Node last;

    circularLL()
    {
        start=null;
        last=null;
    }

    class Node
    {
        int data;
        Node next;

        Node(int datanew)
        {
            data=datanew;
        }
        public void setNext(Node n)
        {
            next=n;
        }
        public Node getNext()
        {
            return next;
        }
        public void setData(int datanew)
        {
            data=datanew;
        }
        public int getData()
        {
            return data;
        }
    }

    public void insert(int datanew)
    {
        Node p=new Node(datanew);
        if(start==null)
        {
            start=p;
            last=p;
            p.setNext(p);
        }
        else
        {
            last.setNext(p);
            last=last.getNext();
            last.setNext(start);
        }
    }
    public void delete(int datanew)
    {
        Node temp=start;
        Node temp1=start;
        Node previous=null;
        Node previous1=null;
        if(last.getData()==datanew)
        {
            while(temp1.getData()!=datanew)
            {
                previous1=temp1;
                temp1=temp1.getNext();
            }
            previous1.setNext(temp.getNext());
            last=previous1;
        }
        while(temp.getData()!=datanew)
        {
            previous=temp;
            temp=temp.getNext();
        }
        previous.setNext(temp.getNext());
    }

    public void display() {
        int count = 0;

        if(start == null) {
            System.out.println("\n List is empty !!");
        } else {
            Node temp = start;

            while(temp.getNext() != last) {
                System.out.println("count("+count+") , node value="+temp.getData());
                count++;
                temp = temp.getNext();
            }
            System.out.println("count("+count+") , node value="+temp.getData());
        }
    }

    public static void main(String args[])
    {
        circularLL ll=new circularLL();
        ll.insert(40);
        ll.insert(20);
        ll.insert(22);
        ll.insert(15);
        ll.insert(16);
        ll.insert(35);
        ll.insert(40);
        ll.insert(38);
        ll.delete(22);
        ll.display();
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ You would get more reviews if you provide more information about this piece of code. \$\endgroup\$
    – Malachi
    Dec 31, 2014 at 15:37

2 Answers 2

11
\$\begingroup\$

I'm not a big algorithms/data structures guy, but the only thing that caught my attention was the delete function. Is there a reason that delete uses data to identify the node, as opposed to just taking in the Node object you want to delete? It seems like that could get hairy if two nodes have the same data (which happens in main).

Besides that, there are a few comments I have in general about the code. Most of the following is fairly standard code format type stuff, but I think it's worth saying it as many times as possible.

Formatting

It's fairly minor, it helps to make sure there's an empty line between methods.

Also, I know some feel differently, but my preference about operators is that they have spaces on either side. data=datanew vs data = datanew.

Naming

In Java, the convention is that class names start with a capital letter, so circularLL should be CircularLL. Even better, it should really be something more descriptive like CircularLinkedList.

Besides that, names of methods, members, variables, etc should follow lowerCamelCase. So, datanew becomes dataNew, but I would personally call it newData.

In delete, avoid variable names like temp1 and previous1. With the number of variables and how they're named, you're starting to enter mental-juggling territory. What I mean by that is that as I read through the code, I have to keep reading back through the code to remember what a variable does because its name isn't descriptive enough.

Etc

I don't have too much else to say, but I would consider breaking this code out into two or three files. I would make a new class with main to separate the linked list from what's using it, and I might put Node in its own class.

Sorry I couldn't help out more with the efficiency/number of variables issue, but you don't seem to be doing anything too crazy, and like I said before, data structures and algorithms aren't my strong suit. Just thought I'd throw in my two cents.

\$\endgroup\$
2
  • \$\begingroup\$ Oh, just saw that you indented it, I'll take that out. \$\endgroup\$
    – boztalay
    Jan 9, 2014 at 5:34
  • \$\begingroup\$ +1 for formatting. The code looks closer to C code instead of java. While old, the code conventions from Sun is pretty widely accepted: oracle.com/technetwork/java/javase/documentation/… \$\endgroup\$
    – pgreen2
    Jul 21, 2014 at 19:31
3
\$\begingroup\$

Generics would be quite useful here, because your List only works with integers, so your classes could be reworked to take type parameters

 public class CircularLL<T>{

 Node<T> start;
 Node<T> last;

 class Node<T>{
  T data;
  public void setData(T datanew)
  {
    data= datanew;
  }
  public T getData()
  {
    return data;
  }
  ....
 }

}

and now you create Lists of different types.

  CircularLL<Integer> list = new CircularLL<Integer>();
  CircularLL<String> list = new CircularLL<String>();

in Java 7 you got type inference

  CircularLL<Integer> list = new CircularLL<>();

Please note, that CircularLL is not the best name for this class, I would call it CircularLinkedList instead.

One more thing, initialising instance variables to null is useless because it's done by default.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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