2
\$\begingroup\$

Here is my implementation of a Linked List. I think it should be efficient because of the head node and rear node tracking. Please give any feedback on my implementation of Linked List.

import java.util.Scanner;

public class LinkedList{

  private static LinkedList head;
  private static LinkedList rear;
  private LinkedList next;
  private Object data;

  public LinkedList(Object data,LinkedList next ){
   this.next = next;
    this.data = data;

  }

  public LinkedList(){
    this.head = new LinkedList(null);
    this.next = null;
    this.data = null;
  }



  public LinkedList(Object data){
    this.next = null;
    this.data = data;
  }

 public Object getData(){
   return this.data ;
 }

  public void setData(Object data){
    this.data = data;
  }

  public LinkedList getNext(){
    return this.next;
  }

  public void SetNext(LinkedList next){
    this.next = next;
  }

  public void add(Object data){



    if(head.next == null){

      head.next = new LinkedList(data);
      rear = head;
    }

    else{
      LinkedList temp = head.next;
        while(temp.next != null){
          temp = temp.next;
        }
      temp.next = new LinkedList(data);
      rear = temp.next;
    }
    }

  public void remove(Object data){

    LinkedList previous = new LinkedList(null);
    LinkedList temp = new LinkedList(null);

    if(head.next == null){
      System.out.println("There is nothing to remove from the list");
      return;
    }
    temp = head;

    while(temp.next != null){
      previous = temp;
      temp = temp.next;
      if(temp.data == data){
        if(temp.next == null){
          rear = previous;
        }
        previous.next = temp.next;
        return;
      }



    }

    System.out.println("That data is not in the list");




  }




  public void printAll(){

    if(head.next == null){
      System.out.println("The list is empty. There is nothing to print");
      return;
    }

      LinkedList temp = head.next;
      if(temp.data != null){
        System.out.println(temp.data);

      }
      else{
        System.out.println("empty data link\n\n");
        return;}


      while(temp.next != null){

        temp = temp.next;

        if(temp.data != null){
        System.out.println(temp.data);
      }
      else{

        System.out.println("empty data link\n\n");
      }

      }

      return;




  }

  public void addFirst(Object data){

    if(head.next == null){

      head.next = new LinkedList(data);

    }

    else{

      LinkedList temp = new LinkedList(data);
      temp.next = head.next;
      head.next = temp;



    }

  }

  public void addLast(Object data){

    if(rear == head){

      addFirst(data);
    }

    else{
      rear.next = new LinkedList(data);
    }


  }


  public static void main(String args[]){

    LinkedList list = new LinkedList();
    Scanner scanner = new Scanner(System.in);

    while(true){
      System.out.println("\nEnter a command: \n1. add [data]\n2.remove [data]\n3.printall\n\n");


      String command = scanner.next();

      if(command.equalsIgnoreCase("add")){

        list.add(scanner.nextInt());

      }
      if(command.equalsIgnoreCase("remove")){

        list.remove(scanner.nextInt());

      }
      if(command.equalsIgnoreCase("printall")){

        list.printAll();

      }

    }

  }



  }
\$\endgroup\$
1
  • \$\begingroup\$ Welcome to code review. You might want to change the title to just "Linked List Implementation in Java" and ask the efficient question within the body of the question. Some tips on asking good questions are at this webpage: codereview.stackexchange.com/help/how-to-ask. You want to draw attention to your question to get more answers. \$\endgroup\$
    – pacmaninbw
    Jul 21, 2016 at 13:39

3 Answers 3

2
\$\begingroup\$

Static

The thing that stands out most about your implementation is that you're using static members for the head and rear of your list. This means that they're shared amongst all instances of the LinkedList class. In effect, this means that you can only have one list in your program which is a limitation that you probably want to avoid.

Separation of concerns

Typically, there is a separation between the items within a list and the list itself. In your implementation, Your LinkedList class is not only responsible for operations that one might perform on a list, like remove, it is also responsible for storing data for a specific link within the list. This is confusing. Thing about creating a Node or Link class that represents a single link/node in the chain and is responsible for storing a pointer to the next Link/Node and the data for this link. Then your LinkedList class becomes responsible for link management, not direct data storage.

Prints Vs Exceptions

Rather than methods like remove print to the console, you might want to consider throwing exceptions to indicate that the client of the class has attempted to do something invalid. Currently, as remove returns void, there's no way for the calling code to know that it's tried to do something invalid.

Printall

Printall doesn't really belong in a list implementation (although it's often added when the list is being written for learning purposes). Providing iteration methods should be enough for the client to perform printing if required. That way the list can concentrate on being a list and the client can concentrate on interacting with the user if they want to.

Object Vs Generics

Do you really want to allow multiple objects that are unrelated to be stored in your list? If not, then rather than having a list of type Object, you'd be better off looking into writing it as a generic list so that it can store items in a type safe way.

\$\endgroup\$
2
\$\begingroup\$

Small things

Styling

You should remove empty lines between commands. They are polluting the code.

Hide implementation details

Avoid public modifier on implementation details.

/*
    it is not good for a public API
    <hide-this>
 */
    public Object getData() {
        return this.data;
    }


    public void setData(Object data) {
        this.data = data;
    }

    public LinkedList getNext() {
        return this.next;
    }

    public void SetNext(LinkedList next) {
        this.next = next;
    }
// </hide-this>

Substitute while to for

Prefer

    for (LinkedList temp = head; temp.next != null; temp = temp.next) 

Instead of

    LinkedList temp = head;
    while (temp.next != null) {
        // ...
        temp = temp.next;

Now, the important things

Remove superfluous method

The add method is superfluous. You can decide by one default behavior: addLast or addFirst. By the way, addLast is not working.

Keep head without data

The common implementation of LinkedList is to keep a head node without data. This would keep code simpler. You can always trust that head would be there.

Simplify as much as possible

Compare...

  public void addFirst(Object data){

    if(head.next == null){

      head.next = new LinkedList(data);

    }

    else{

      LinkedList temp = new LinkedList(data);
      temp.next = head.next;
      head.next = temp;



    }

  }

...with...

public void addFirst(Object data) {
        LinkedList temp = new LinkedList(data);
        temp.next = head.next;
        head.next = temp;
}

Reviewed version

private final static LinkedList head = new LinkedList(null);
private static LinkedList rear = head;

private LinkedList next;
private Object data;

public LinkedList() {
    this(null);
}


public LinkedList(Object data) {
    this.next = null;
    this.data = data;
}

public void remove(Object data) {
    if (head.next == null) {
        System.out.println("There is nothing to remove from the list");
        return;
    }
    LinkedList previous = head;
    for (LinkedList temp = head.next; temp != null; temp = temp.next) {
        if (temp.data.equals(data)) {
            if (temp.next == null) {
                rear = previous;
            }
            previous.next = temp.next;
            return;
        }
        previous = temp;
    }
    System.out.println("That data is not in the list");
}

public void addFirst(Object data) {
        LinkedList temp = new LinkedList(data);
        temp.next = head.next;
        head.next = temp;
}

public void addLast(Object data) {
    rear.next = new LinkedList(data);
    rear = rear.next;
}
\$\endgroup\$
0
\$\begingroup\$

The finer points were already explained, but the crucial point is the following:

There should be one data structure for the entire LinkedList, and internally an other data structure representing a single link in the list, often called Node.

You have the entire linked list as static variables (so there is one single list available globally), and LinkedList plays more than one role, both Link and List access.

\$\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.