1
\$\begingroup\$

This code is working fine and I would like for it to be reviewed. Am I using extra objects and variables to store values?

LinkedList

package lab5;

public class LinkedList {

    public static void main(String argsp[]) {
        List ob = new List();

        ob.addAtStart("y", 6);
        ob.addAtStart("w", 4);
        ob.addAtStart("z", 3);

        ob.addAtEnd("a", 3);
        ob.addAtEnd("b", 4);
        ob.addAtEnd("c", 5);

        /*
         * ob.display(); System.out.println("Deleted first one");
         * ob.deleteFromStart();
         */
        ob.display();
        System.out.println("Deleted End one");
        ob.deleteFromEnd();
        ob.display();
    }
}

List

package lab5;

public class List {

    Node head;

    public List() {
        head = null;
    }

    public List(Node e) {
        head = e;
    }

    Node oldfirst = null;
    Node lasthead = null;

    public void addAtStart(String name, int age) {
        Node newObject = new Node(name, age);
        newObject.next = head;

        if (oldfirst == null) {
            oldfirst = newObject;
        }
        head = newObject;
        lasthead = head;

    }

    public void display() {
        Node store = head;
        while (store != null) {
            store.display();
            store = store.next;
            System.out.println();
        }
    }

    public void addAtEnd(String name, int age) {
        Node atEndValue = new Node(name, age);
        oldfirst.next = atEndValue;
        oldfirst = atEndValue;
    }

    public void deleteFromStart() {
        if (head.next != null) {
            head = head.next;
        }
    }

    public void deleteFromEnd() {
        Node start = head;
        Node prev = null;
        while (head != null) {
            prev = head;
            head = head.next;
        }
        prev.next = null;
        head = prev;
    }

    public Node search(String name) {
        return head;
    }

    public boolean isEmpty() {
        return head == null;
    }

    public int size() {
        return (head.toString()).length();
    }
}

Node

package lab5;

public class Node {

    String name;
    int age;
    Node next;

    public Node() {
        name = "Abc";
        age = 10;
        next = null;
    }

    public Node(String name, int age) {
        this.name = name;
        this.age = age;
        next = null;
    }

    public void display() {
        System.out.println("Name: " + name + " Age: " + age);
    }
}
\$\endgroup\$
6
  • \$\begingroup\$ Your post seems fine here, but it's not clear what you're actually asking? What is your question? Please read our on-topic and how-to-ask pages. \$\endgroup\$
    – janos
    Nov 6, 2016 at 18:37
  • \$\begingroup\$ I was told by few people your code need review. Am I using extra objects and variable to store values? \$\endgroup\$ Nov 6, 2016 at 18:45
  • 3
    \$\begingroup\$ We get many linked-list questions here. It would help if you included some context about what this code is expected to accomplish. If it's a homework question, then you should include the instructions too. \$\endgroup\$ Nov 6, 2016 at 18:47
  • \$\begingroup\$ Yes this is homework task. \$\endgroup\$ Nov 6, 2016 at 18:49
  • \$\begingroup\$ Can I ask why I get -1 ? \$\endgroup\$ Nov 8, 2016 at 7:43

1 Answer 1

1
\$\begingroup\$

Don't leave code lying about

When you're done with code, delete it, don't just comment it out. It adds unnecessary noise to your code and makes it look messy.

Unused code / Construction from Node

You don't currently seem to use this constructor:

public List(Node e) {
    head = e;
}

How likely are you to use it? What's going to have constructed the Node / Node chain that you're assigning into your list. If it's not needed get rid of it. If it is needed, then consider if you really want the Node to be shared between the list and it's caller.

oldfirst / bug

This is a very odd variable name at a class level. It sounds like something you might use as a temporary variable in a method, for example when removing/replacing the head of the list. It's usage is also rather odd. For example in addAtStart you do:

if (oldfirst == null) {
    oldfirst = newObject;
}

So, you only assign to it the first time you add an item?!?

This also seems to result in a bug, where if you addAtEnd before you've addAtStart, you will do:

Node atEndValue = new Node(name, age);
oldfirst.next = atEndValue;
oldfirst = atEndValue;

Until you call addAtStart, oldfirst is null, so oldfirst.next is going to crash...

Naming

Think about what variables you're declaring at a class level and what their purpose is, then give them names that reflect that. It will be a lot easier to follow your code and identify bugs. In this instance, it seems like perhaps oldfirst is actually supposed to pointing at the tail/end of the list, rather than the head.

Consider what you're supposed to do to any class variables (such as oldfirst) when you delete items from the list.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you! you give me time and response to my code. Inshallah! I will in future code to abstain these faults. Moreover, I accept I have bad smell in my code that's why I recommend by someone to post here. I learnt 3 things today from you. Thank you ! @forsvarir \$\endgroup\$ Nov 8, 2016 at 7:42

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.