4
\$\begingroup\$

I am a CS student and I was wondering is this code looks clean. I tested it and it passed every tested but I think the delete(Node node) is a bit long, I did not find a way to reduce it

Node class

public class Node {
      private int data;
      private Node next;
      Node(int data){
        this.data = data;
      }

      public int getValue(){
        return this.data;
      }

      public void setNext(Node next){
        this.next = next;
      }

      public Node nextNode(){
        return this.next;
      }
    }

LinkedList class

public class LinkedList {
  Node head;
  Node tail;
  int size = 0;


  public void add(Node node){
    if(tail == null && head == null){
      tail = node;
      head = node;

    } else{
      tail.setNext(node);
      tail = tail.nextNode();
    }
    size ++;
  }

  public int getSize(){
    return this.size;
  }
  public int getTail(){
    if(tail == null){
      return -1;
    } else {
      return tail.getValue();
    }
  }

  public void delete(Node node){
    if(isPresent(node)){
      if(size == 1){
        this.tail = null;
        this.head = null;
        size = 0;
      }
      else if(node == head){
        head = head.nextNode();
        size --;
      }

      else if(node == tail){
        Node iterator = head;
        while(iterator.nextNode().nextNode() != tail){
          iterator = iterator.nextNode();
        }
        tail = iterator.nextNode();
        iterator.nextNode().setNext(null);
        size --;
      }

      else{
        Node iterator = head;
        while(iterator.nextNode() != node){
          iterator = iterator.nextNode();
        }
        iterator.setNext(iterator.nextNode().nextNode());
        size --;
      }
    }
  }

  public int getHead(){
    if(head == null){
      return -1;
    } else {
      return head.getValue();
    }
  }


  public void printList(){
    Node iterator = head;

    while(iterator != null){
      System.out.println(iterator.getValue());
      iterator = iterator.nextNode();
    }
  }

  public boolean isPresent(Node node){
    if(head == null){
      return false;
    }

    else if(head.getValue() == node.getValue()){
      return true;
    }

    else {
      Node iterator = head;
      while(iterator != null){
        if(iterator.getValue() == node.getValue()){
          return true;
        }
        iterator = iterator.nextNode();
      }
      return false;
    }
  }
}
\$\endgroup\$
3
\$\begingroup\$

Indeed the cases for delete can be reduced.

  public void delete(Node node) {
    Node prior = null;
    Node current = head;
    while (current != node && current != null) {
      prior = current;
      current = current.next;
    }
    if (current != null) {
      if (prior == null) {
        head = current.next;
      } else {
        prior.next = current.next;
      }
      --size;
      if (current == tail) {
        tail = prior;
      }
    }
  }

In general one would keep Node an internal non-public class, for instance one would not like people to play with the next field. Hence I would propose:

  public void delete(int value) {
    Node prior = null;
    Node current = head;
    while (current != null && current.value != value) {
      prior = current;
      current = current.next;
    }
    if (current != null) {
      if (prior == null) {
        head = current.next;
      } else {
        prior.next = current.next;
      }
      --size;
      if (current == tail) {
        tail = prior;
      }
    }
  }

For the rest:

  • A tab of 4 spaces is still my favorite, probably still mainstream.
  • iterator is an unlucky naming, as there exist an Iterator in java, with a collection method iterator(). I used current.

And (as goodie) what about System.out.println(list);:

@Override
public String toString() {
    StringBuilder sb = new StringBuilder("[");
    Node current = head;
    while (current != null) {
      if (sb.length() >= 1) {
          sb.append(", ");
      }
      sb.append(current.value);
      current = current.next;
    }
    sb.append("]");
    return sb.toString();
}
|improve this answer|||||
\$\endgroup\$
3
\$\begingroup\$

As has been pointed out by @Joop Eggen, you should decide if Node is part of the interface or not. As it stands, you can directly access the value of head and tail via the accessors (getHead,getTail), however you can't get at any of the other values in the other values you've stored in the LinkedList unless you happen to have kept track of the Node that you inserted into it. Try writing printList in a client class for your linked list.

  public void setNext(Node next){
  public Node nextNode(){

These methods represent a property, so they'd usually be named consistently, i.e. getNext, setNext. However, if you go down the route of making Node an internal class, I'd get rid of them altogether and directly access the fields.

It's also worth mentioning that at the moment, even though the list stores integers, you can't safely store -1 in it because you're using this as an error value when returning head/tail for an empty list. It'd be better to throw an exception in such a situation, or return an Optional<Integer>.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ OptionalInt rather. I thought of it too. Your more elaborate answer really is worthwile reading. \$\endgroup\$ – Joop Eggen Jan 21 at 11:31

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.