-3

To practice using pointers in c++ (coming from experience of c#), I decided to try and write my own linked list class. However using classes and objects seems to do some wacky behaviour that I do not understand.

class Node
{
    public:
    int value;
    Node* Next = NULL;

    Node(int Value)
    {
        this->value = Value;
        this->Next = NULL;
    }
};

class LinkedList
{
    public:
    Node* Head = NULL;
    Node* Current = NULL;
    int count = 0;

    LinkedList()
    {
        Node n(-1);
        Head, Current = &n;
    }

    void pushNode(int value)
    {
        //Did try stuff here, but display() caused issues.
    }

    void display()
    {
        Node* curr = Head;
        cout << "HEAD->";

        while(curr->Next != NULL)
        {
            curr = curr->Next;
            cout << curr->value << "->";
        }

        cout << "NULL";
    }
};

int main() {

    LinkedList ml;
    ml.pushNode(10);
    ml.display();

    return 0;
}

Upon running this code "HEAD->" is written to the console, and then the program ends. If I added an item to the list the console would be spammed with whatever the value was with no end. HELP!

πάντα ῥεῖ
  • 1
  • 13
  • 116
  • 190
  • Can you point to the line in your `display` function that you think will print the contents of the _first_ node in the list? You call `curr = curr->Next;` before you ever print `curr->value`. – Nathan Pierson Mar 19 '23 at 14:56
  • 1
    `Node n(-1); Head, Current = &n;` This does two things. First, `Head` is not modified at all (what you wrote doesn't mean "assign the same value to both `Head` and `Current`" as you appear to believe), and is still null. Worse, `Current` is set to point to a local variable. When the constructor returns, that variable goes out of scope and is destroyed; `Current` becomes a dangling pointer, any attempt to use it later would exhibit undefined behavior. – Igor Tandetnik Mar 19 '23 at 14:57
  • 1
    Other issues with your code: `Head, Current = &n;` is a bad idea on several levels. First, it only assigns to `Current`, not `Head`, because the [built-in comma operator](https://en.cppreference.com/w/cpp/language/operator_other#Built-in_comma_operator) doesn't work the way you seem to expect. But also assigning `&n` to either of those is a terrible idea because `n` is a local variable that goes out of scope as soon as the constructor returns, so you have a dangling reference. – Nathan Pierson Mar 19 '23 at 14:58
  • And since `Head` is still null after the constructor, `Node* curr = Head; curr->Next` in `display` attempts to dereference a null pointer, thereby exhibiting undefined behavior. Most likely, the program crashes at this point. – Igor Tandetnik Mar 19 '23 at 14:59
  • Anyway, your implementation of `pushNode` probably also has problems, but we can't see what any of them are because you omitted that code. – Nathan Pierson Mar 19 '23 at 15:00
  • *I decided to try and write my own linked list class* -- Well, good luck with this, since I have yet to see a C++ programmer with little to no experience in C++ actually create a fully working linked list class. – PaulMcKenzie Mar 19 '23 at 15:51
  • 1
    Your experience with C# is actually going to be a significant hinderance to understanding C++ since the languages are so different. Needless to say you have made beginner errors in the code posted above. Your best bet would be to forget about C# and learn C++ from scratch using a reputable source such as a [good book](https://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list/388282) – john Mar 19 '23 at 16:40
  • Your code is an example of [why should one always enable compiler warnings?](https://stackoverflow.com/questions/57842756) *(in particular "warning: left operand of comma operator has no effect" and "warning: storing the address of local variable 'n' in '*this.LinkedList::Current'")* – JaMiT Mar 19 '23 at 20:03

2 Answers2

0

C# uses garbage collection, which alleviates the programmer from a lot of concerns about managing memory.

C++ does not provide garbage collection, so the programmer needs to take care to manage the memory of objects and their lifecycles. Modern C++ provides smart pointers (like std::unique_ptr) and containers (like std::vector, although which was not used in the code example) to help manage memory and object lifecycles.

#include <iostream>
#include <memory>
#include <utility>

using std::cout;
using std::make_unique;
using std::move;
using std::ostream;
using std::unique_ptr;

class Node {
public:
    int value{};
    unique_ptr<Node> next;

    Node(int value_, unique_ptr<Node> next_) : value{value_}, next{move(next_)} { }
};

class LinkedList {
public:
    unique_ptr<Node> head;

    LinkedList() { }

    void pushNode(int value) {
        auto node{ make_unique<Node>(value, move(head)) };
        head = move(node);
    }

    void display(ostream& out) {
        Node* curr = head.get();
        out << "HEAD->";

        while (curr) {
            out << curr->value << "->";
            curr = curr->next.get();
        }

        out << "null\n";
    }
};

int main() {
    LinkedList ml;
    ml.pushNode(10);
    ml.pushNode(20);
    ml.pushNode(30);
    ml.display(cout);
}
Eljay
  • 4,648
  • 3
  • 16
  • 27
-1

Thank you everyone! I've addressed your comments and done some more research and resulted in the following.

#include <iostream>
using namespace std;

class Node
{
    public:
    int value;
    Node* Next = nullptr;
    Node(int data)
    {
        value = data;
    }
};

class LinkedList
{
    private:
    Node* Head = nullptr;
    Node* Current = nullptr;
    int count = 1;

    public:
    LinkedList(int data)
    {
        Head = new Node(data);
        Current = Head;
    }

    void push(int value)
    {
        Node* n = new Node(value);
        Current->Next = n;
        Current = n;
        count++;
    }

    void pop()
    {
        if(count > 1)
        {
            Node* Temp = Head;
            Head = Head->Next;
            delete Temp;
        }
    }


    void printList()
    {
        Node* curr = Head;
        cout << "HEAD->" << curr->value << "->";

        while(curr->Next != nullptr)
        {
            curr = curr->Next;
            cout << curr->value << "->";
        }

        cout << "NULL" << endl;
    }
};

int main() {

    LinkedList ml(10);
    ml.push(10);
    ml.push(20);
    ml.printList();
    ml.pop();
    ml.printList();

    return 0;
}