4
\$\begingroup\$
  1. I had implemented a simple Linked list Insert, print and insert at the nth position. It works fine.
  2. Could someone review code and let me know where I could have done better (or) where I had gone wrong?

class Linkedlist
     {
       public Nodes current;
       public static Nodes headnode;
       public static int nodecount;
       public void Addatfirst(object d)
      {

          if (headnode == null)
          {
              headnode = new Nodes(d);
              nodecount++;
           }
          else
          {
              Nodes dummy = new Nodes(d);
              dummy.Next = headnode;
              headnode = dummy;
              nodecount++;
           }
         }

     public void Print()
    {
        current = headnode;

        while (current.Next != null)
        {
            Console.Write("|" + current.data + "| -> ");
            current = current.Next;
            if (current.Next == null)
            {
                Console.Write("|" + current.data + "| -> ");
            }
        }
    }

    public void AddatLast(object d)
    {
        current = headnode;
        while (current.Next != null)
        {
            current = current.Next;
            if (current.Next == null)
            {
                current.Next = new Nodes(d);
                nodecount++;
                return;
            }
        }
    }

    public void AddatNthPosition(object d, int position)
    {
        current = headnode;
        for (int i = 1; i < position; i++)
        {
            current = current.Next;
            if (position - i == 1)
            {
                var temp= new Nodes(d);
                temp.Next = current.Next;
                current.Next = temp;
            }
        }
    }
}
public class Nodes
{
    public object data;
    public Nodes Next;
    public Nodes(object d)
    {
        data = d;
    }
}
\$\endgroup\$
  • 11
    \$\begingroup\$ Calling a Node a Nodes is not correct English and really impairs reading this. Multiplicity matters (a lot) in programming. \$\endgroup\$ – Henk Holterman May 24 '17 at 8:25
  • 1
    \$\begingroup\$ I know using Nodes instead Node is absurd. I also had another "Node" class with which i was playing around. Laziness is the problem and thanks for the feedback @Henk Holterman. I will keep that in mind going forward. \$\endgroup\$ – Vishwa May 24 '17 at 16:24
10
\$\begingroup\$

There are actually several bugs in your code. Your list is also not very easy to use because it can't be iterated.

Bugs

  • static fields are shared among class instances. So if you create multiple Linkedlist instances, they will all use the same headnode and nodecount. That is obviously not what you want. Those fields should not be static.
  • AddatNthPosition doesn't add anything if you pass 0 or 1 as index. Also, it throws a NullReferenceException when given an index that is too large. Throwing an ArgumentOutOfRangeException would be better if the index is too small (negative) or too large.
  • Only Addatfirst takes into account that the list can be empty. The other add methods fail with a NullReferenceException when called on an empty list.

Problems

  • Your list cannot be iterated because it does not implement IEnumerable. That makes it difficult to use: instead of using foreach or Linq, calling code is forced to manually walk through its nodes. While you're at it, implementing ICollection (count) and IList (indexing, adding, inserting, removing) may also be useful.
  • public fields can be modified by other code. That means that some other code could replace the head node of a linked list, or adjust its count. That can easily cause bugs or other unexpected behavior. Classes should hide their internal state and implementation details.
  • Print may be useful for debugging, but it should not be part of your (final) list class. Classes should (ideally) have only a single responsibility, and 'storing items' and 'displaying things' are different things as far as I'm concerned.
  • Why is there a current field? It looks like that should be a local variable in some of your methods.

Improvements

  • Consider making your class generic. This improves type safety and makes your list easier to work with.
  • Try sticking to the official naming conventions to make your code easier to read. C# code uses PascalCase for class and method names: LinkedList instead of Linkedlist, AddToStart instead of AddtoStart, and so on. camelCase is used for field names: headNode instead of headnode, nodeCount instead of nodecount, and so on.
  • Try using more descriptive names. value instead of d, newNode instead of dummy, and so on. Also, Nodes is plural, but it represents a single node, so it should be named Node instead.
  • Look at the collections in the standard library. They often use particular names for their methods, and that's what other programmers will be familiar with. Using different names makes your code harder to use. Consider renaming AddatLast to Add, and combining Addatfirst and AddatNthPosition into a single Insert method (where its first argument is the index, conform the IList interface). Also rename nodecount to Count and make it a get-only property, so it can't be modified by other code.
  • A Remove and/or RemoveAt method would make your list more useful.
  • If adding items at the end is important, consider adding a lastNode field to improve performance for that case.
  • Inconsistent indentation makes code harder to read, and it looks a little sloppy.
\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for taking time and providing feedback, appreciate it. I will take your suggestion and will improve. \$\endgroup\$ – Vishwa May 24 '17 at 16:30
6
\$\begingroup\$

public fields are bad practice, public static fields are even worse! A static headnode means that ALL your LinkedLists share the same Node as headnode.

You should keep private the internal state of your classes, see https://en.wikipedia.org/wiki/Encapsulation_(computer_programming)

Instead of a list of objects you should create a list of generic objects. Your Nodes class (I'd rename it to Node) could be rewritten as

public class Node<T>
{
    public T Data { get; set; } 

    public Node<T> Next { get; set; }

    public Node(T data)
    {
        Data = data;
    }
}

(Implementation could be still improved)

Update: I'd move the Node class inside the LinkedList.

\$\endgroup\$
  • \$\begingroup\$ There is a guideline against public nested types, so unless you want to remove the (concept of a) Node completely from the interface I wouldn't move it inside. \$\endgroup\$ – Henk Holterman May 24 '17 at 9:49
  • \$\begingroup\$ @HenkHolterman In my opinion the user of a LinkedList should not be able to manipulate the Nodes directly so I'd hide it. \$\endgroup\$ – user131519 May 24 '17 at 10:00
  • 2
    \$\begingroup\$ That is valid but then stress that it should be a private type. \$\endgroup\$ – Henk Holterman May 24 '17 at 10:05
  • \$\begingroup\$ As a side note, in the majority of cases you'd use a public static for you'd probably want to use an enum for. I don't like the idea of explaining why public static is a bad idea unless you explain it. There's nothing inherently wrong with public static - in some code it's used to expose flags. Though, yeah, in this particular answer it's bad. \$\endgroup\$ – Dan Pantry May 24 '17 at 22:11
  • \$\begingroup\$ @DanPantry The problem is public static Nodes headnode means that ALL the LinkedLists created will share the same Nodes. I guess I should better explain this in my answer. \$\endgroup\$ – user131519 May 25 '17 at 8:01
5
\$\begingroup\$

I have a few suggestions but keep in mind I'm a student still, not a 10+ year of experience professional.

  • Commentary/documentation. It's a good idea, even in your own private project to have the habits to write documented code. (but maybe you posted the code without them to reduce the length of your post)

  • When you implement a data structure, or well-known algorithms, try to use the standard name for it. In C#, AddLast is the name of the method adding an element to the end, not AddatLast (and I would name it AddAtLast if you don’t want to use AddLast but that's my taste).

  • Instead of print, I'd implement a toString method, which you can use to print, but gives you more flexibility.

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

In addition to the sound advice provided,

Avoid redundancy

if (headnode == null)
{
   headnode = new Nodes(d);
   nodecount++;
}
else
{
   Nodes dummy = new Nodes(d);
   dummy.Next = headnode;
   headnode = dummy;
   nodecount++;
}

nodecount++ immediately strikes out as something that should be outside of the conditionals blocks since it happens regardless of control flow.

Be aware of algorithm complexity

Much of the appeal of a linked list is offering constant time insertion and deletion from both ends of the list. You do have the ability to do head operations such as prepend well, however anything to the tail end requires you to iterate through the entire list. Adding a Tail Node and having a prev field for Node would allow you append and remove from tail in O(1) rather than the current linear time O(n) operations.

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