C++ Node and Linked List syntax

Question

I have a problem with this code. I'm fairly new to C++ however most of it is already easy to understand. I have tried making a simple linked list data structure, however, it prints garbage and not the values inside the list. My question is, where did I go wrong in my syntax to have it display the addresses?

Output: enter image description here

class Node
{
  public:
    int data;
    Node *next;

    Node(int data)
    {
      data = data;
      next = NULL;
    };
};

class LinkedList
{

    Node *first;
    Node *last;
    int count;

public:
    LinkedList()//constructor for the LinkedList
    {
        //initialization 
        first = NULL;
        last = NULL;
        count = 0;
    };
    void AddItem(int data)
    {
        Node *newItem = new Node(data);

        if(first == NULL)
        {
          first = newItem;
          last = newItem;
        }
        else
        {
          Node *traversal = first;
          while(traversal->next != NULL)
          {
              traversal = traversal->next;
          }
          traversal->next = newItem;
          last = traversal->next;
        }
        count++;
    }

    void DisplayList()
    {
        cout<<endl;
        Node *traversal = first;
        while(traversal->next != NULL)
        {
            cout<<"["<<traversal->data<<"] ";
            traversal = traversal->next;

            if(traversal == NULL)
            {
                break;
            }
        }
    }
    bool isEmpty()
    {
        if(count < 1)
        {
            cout<<"List is empty";
            return true;
        }
        else
        {
            cout<<"List is not empty";
            return false;
        }
    }
};

int main()
{
cout <<"Linked Lists demo"<<endl;


LinkedList collection;
collection.AddItem(1);
collection.AddItem(3);
collection.AddItem(5);
collection.AddItem(7);
collection.AddItem(9);
collection.AddItem(11);
collection.isEmpty();
collection.DisplayList();
cin.get();

Please do try to debug and put breakpoints to check various assignments at run time. — abnvp, Feb 27 '14 at 06:34
Your member variable names look like ordinary variable names and so you fell into a very common trap. A common convention is to prefix member variables with m_ e.g m_data. Try this with your code and then review Node's constructor — kfsone, Feb 27 '14 at 07:20

Remy Lebeau · Accepted Answer · 2014-02-27T06:29:49.263

5

Node(int data)
{
    data = data; // <-- incorrect
    next = NULL;
};

You are not assigning the input parameter to the Node::data member. You are assigning the input parameter back to itself, leaving the Node::data member to get initialized with whatever random value was already present in the memory block that was used to allocate the new Node. Since the input parameter has the same name as the member, you need to use this->data = data instead:

Node(int data)
{
    this->data = data;
    next = NULL;
};

Or else rename the input parameter so it has a different name:

Node(int value)
{
    data = value;
    next = NULL;
};

Also, since your list has a last member, you can great simplify your AddItem() implementation, you do not need to traverse the list at all (which would take a long time if the list has a lot of items in it):

void AddItem(int data)
{
    Node *newItem = new Node(data);

    if (first == NULL)
        first = newItem;

    if (last != NULL)
       last->next = newItem;
    last = newItem;

    ++count;
}

edited Feb 27 '14 at 06:29

answered Feb 27 '14 at 06:22

Remy Lebeau

555,201
31
458
770

Or better yet, follow the increasingly common practice of prefixing members with "m_", e.g "m_data" (likewise s_ for statics and g_ for globals) – kfsone Feb 27 '14 at 07:17
1

@kfsone: That certainly doesn't qualify as better, and as far as I know was common primarily in MFC, and has (thankfully) been dying out as superior alternatives to MFC have taken over. – Jerry Coffin Feb 27 '14 at 07:26
@JerryCoffin You're thinking of hungarian notation; member variable prefixes (or suffixes) aren't hungarian notation (neither apps nor system). The [mgs]_ prefix is, if anything, catching on, although other popular alternatives are, in example form, "mData", "_data" and "data_". Without such a distinction, you have to more consciously code against variable shadowing, e.g. using "this->member" everywhere or renaming function parameter names. Hungarian was about giving you information that modern IDEs now give you; prefixes are basically a form of namespacing. – kfsone Feb 27 '14 at 17:35
@kfsone: I'm not thinking of HN at all. MFC use `m_*` for members, and such, and it sucked. Prefixes are a form of illness. If you need (or even want) an `m_` or `g_` to keep track of member variables or globals, that's a pretty solid indication that your code is a mess. – Jerry Coffin Feb 27 '14 at 17:52
In grayish areas of C++ it's often difficult to avoid a solution that isn't someone's idea of "a mess". Here the problem arises from C++s allowance of shadowing; it *allows* `static int i; static thread_local int i; struct Foo { int i; Foo(int i) : i(i) { this->i = i; for (int i = 0; i < this->i; ++i) {} } };`. I've worked with some very complex systems, seen some very elegant implementations of very hairy problems, and had to fix them at 4am on a New Years' Saturday and I'll stick with writing code that comes back from QA with feature/functionality issues over language-induced bugs. – kfsone Feb 27 '14 at 20:28

score 4 · Answer 2 · answered Feb 27 '14 at 06:44

You've gotten some answers, but all of them seem to be giving the same (bad) advice.

Instead of changing data = data; to something like this->data = data; you should use a member initialization list:

Node (int data) : data(data), next(nullptr) {}

Personally, I'd probably change that a little further, to allow specifying the "next" element for the node as well:

Node(int data, Node *next=nullptr) : data(data), next(next) {}

Inside the body of the ctor, only the parameter is visible using the bare name, so data = data; just assigned the parameter's value back to itself. In the member initializer list, the compiler is smarter (so to speak) and "knows" which data is which, so even though they have the same name, this assigns the value from the parameter data to the member data (and likewise with next).

As an aside: although the ctor having an empty body may initially look a bit strange, you should get used to it. I'd guess the majority of ctors I write any more have empty bodies.

Another (more or less unrelated) aside: I'd also define the Node class inside the LinkedList class (and probably make it private). Nothing outside of the LinkedList itself really needs to know about the Node class.

`but all of them seem to be giving the same (bad) advice`. True. I stand guilty. +1 — axiom, Feb 27 '14 at 07:06
I picked the first response as the answer since it fixed my issue. However your solution works the same as well and the syntax is easier to look at. Thanks. — Karim O., Feb 27 '14 at 07:13

score 0 · Answer 3 · answered Feb 27 '14 at 06:23

0

It is printing garbage values not addresses, In your AddItem method You are not updating data of the node. Add this line it will work

newItem->data=data

answered Feb 27 '14 at 06:23

prince

1,129
1
15
38

score 0 · Answer 4 · answered Feb 27 '14 at 06:35

0

You can also use

Node(int data)
{
  Node::data = data;
  next = NULL;
};

to initialize data which is a member variable of Node class.

answered Feb 27 '14 at 06:35

rajenpandit

1,265
1
15
21

C++ Node and Linked List syntax

4 Answers4