2
\$\begingroup\$

I'm looking for some feedback regarding my implementation of a generic singly-linked list in C#. I'm also looking for some tips on how to implement other techniques such as using IEnumerable or other .NET interfaces I'm currently not aware of.

Node.cs

using System; 
namespace SinglyLinkedLists
{
    public class Node<T>
    {
        private Node<T> _next;
        private T _data;

        public Node<T> next{
            get { return _next; }
            set { _next = value; }
        }

        public T data{
            get { return _data; }
            set { _data = value; }
        }

        public Node(T newData){
            this._data = newData;
            this._next = null; 
        }

        public static void printNodes(Node<T> head){
            Node<T> currentNode = head;
            while (currentNode != null){
                Console.WriteLine(currentNode.data);
                currentNode = currentNode.next;
            }
        }
    }
}

SinglyLinkedLists.cs

using System; 
namespace SinglyLinkedLists
{
    public class SinglyLinkedLists<T>
    {
        private Node<T> head;
        private Node<T> previous;
        private Node<T> current;
        private Node<T> tail;

        private int numberOfNodes = 0;

        public SinglyLinkedLists()
        {
            head = null;
            previous = null;
            current = null;
            tail = null; 
        }

        public void PrintList(){
            Node<T> current = head;
            while (current != null){
                Console.WriteLine(current.data);
                current = current.next;
            }
        }

        public void AddLast(T data){
            Node<T> newNode = new Node<T>(data);
            if (tail == null){
                head = tail = newNode;
            }
            else{
                tail.next = newNode; 
                tail = newNode; 
            }
            numberOfNodes++;
        }

        public void AddLastTwo(T data){
            Node<T> newNode = new Node<T>(data);

            if (head == null){
                head = newNode; 
            }
            else{
                current = head;
                while (current.next != null){
                    current = current.next; 
                }
                current.next = newNode;
            }
            numberOfNodes++;
        }

        public void RemoveNode(T data){
            Node<T> nodeToRemove = new Node<T>(data);
            current = head;

            if(head.data.Equals(nodeToRemove.data)){
                current = head.next;
                head = current;
            }
            else{
                while (current != null){
                    if (current.data.Equals(nodeToRemove.data)) {
                        previous.next = current.next;
                        current = previous;
                    }
                    else{
                        previous = current;
                        current = current.next;
                    }
                }
            }
            numberOfNodes--;
        }

        public int ListSize(){
            return numberOfNodes;
        }

        public bool InList(T data){
            Node<T> nodeToLookFor = new Node<T>(data);
            Node<T> current = head;
            while (current != null){
                if (current.data.Equals(nodeToLookFor.data)){
                    return true;
                }
                current = current.next;
            }
            return false;
        }

    }
}
\$\endgroup\$
  • 6
    \$\begingroup\$ I'm also looking for some tips on how to implement [..] - You should remove this request from your question as it makes it off-topic because we are not implementing new features for you. But if you already know about IEnumerable why don't you add it yourself? \$\endgroup\$ – t3chb0t Mar 23 at 22:25
2
\$\begingroup\$

A few things that stuck out to me:

You don't gain anything by having the Node class public. The user of your linked list should only need to be concerned about the data.

It seems very illogical to me to have the Node class printing the LinkedList. This should more properly be only part of the LinkedList itself. It would probably be advantageous though for the Node class to override the ToString() method to make it easier to print the data.

From a quick perusal, I couldn't see any use for the previous property except in the RemoveNode method. I would suggest keeping it local to that method.

A comment explaining the purpose of the AddLastTwo method. To me it appears to iterate over the list to find the last node, instead of using the last property of the class, like the AddLast.

To me names like AddLast and RemoveNode are over complicated. If a node isn't being added to the end then it would be an insert operation not an add. It shouldn't be necessary to tell the user that it's being added to the end. Likewise the user doesn't need to know that the data is stored in a node, they are simply concerned with removing the data.

When you are using default get and set methods it's much easier to use the default syntax:

public Node<T> next{get;set;}
\$\endgroup\$
1
\$\begingroup\$

This is a small answer but is kind of important in my opinion. bool InList(T data) and int ListSize() are existing patterns among .Net classes, they should be named Contains and Count. The lesson to come out of this is that you should try to reuse common names when the pattern fits.

Also, your class name shouldn't be pluralized. SinglyLinkedLists -> SinglyLinkedList

\$\endgroup\$

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.