Linked Lists

[elflira]

Hello,
I'm trying to create a linked list in c++, but its not working out too well. Any suggestions to where i'm going wrong would be great. Here's what i have so far:


// Node.h

#include <string>

using namespace std;

class Node
{
public:
Node();
Node(string e, Node* n);
void setElement(string e);
string getElement();
void setNext(Node* n);
Node* getNext();
string toString();

private:
Node* next;
string element;
};

Node::Node()
{
element = "";
next = NULL;
}

Node::Node(string e, Node* n)
{
element = e;
next = n;
}

void Node::setElement(string e)
{
element = e;
}

string Node::getElement()
{
return element;
}

void Node::setNext(Node* n)
{
next = n;
}

Node* Node::getNext()
{
return next;
}

string Node::toString()
{
string output = element + "\n";
return output;
}


// SLinkedList.h

#include "Node.h"
#include <string>

using namespace std;

class SLinkedList
{
public:
SLinkedList();
void add(string e);
int getSize();

private:
Node* head;
int size;
};

SLinkedList::SLinkedList()
{
head = NULL;
size = 0;
}

void SLinkedList::add(string e)
{
Node node (e, NULL);
Node* n = new Node;
n = &node;

if (head == NULL)
head = n;

else if(head != NULL)
{
n->setNext(head);
head = n;
}

size++;
}

int SLinkedList::getSize()
{
return size;
}

[firedraco]

You SLinkedList::add() is broken. Currently,you are creating a new temporary node on the stack. Next, you are allocating a new node on the heap. Then, you are assigning that new node pointer to be the original stack node (which gives you a memory leak). Also, once your function ends, your temporary stack node gets destructed, meaning your node pointer is now pointing at bad data.

FYI, your else if section could be changed to just a else.

[elflira]

First of, thanks for the quick reply.
The thing is, i'm new to c++ and still not used to pointers (i'm more of a JAVA programmer which does not include pointers in the language). I sort of understood your main point, but still can't seem to fix it. Any ideas?

[rollie]

Firedraco is correct; I believe you need to change

Node node (e, NULL);
Node* n = new Node;
n = &node;

to

Node* n = new Node(e, NULL)

Also, for simplification,

if (head == NULL)
head = n;

else if(head != NULL)
{
n->setNext(head);
head = n;
}

can be reduced to

if (head != NULL)
{
    n->setNext(head);
}

head = n;

--Rollie

p.s., please use the code tags when posting code! indentation wouldn't hurt either :)

[elflira]

Thanks its working now. Oh and sorry for not using the tags :$ im new to this forum but i will use them from now on :)

[filipe]

Is there any reason to use if(head != NULL) rather than if(head) ?

[firedraco]

Not unless you like being explicit (or whatever you are comparing like that isn't convertible to a bool).

[kempofighter]

If you are new to C++ my suggestion would be to learn how to use the std::list that is already provided by the language. Reinventing the wheel is not a good idea, especially if you are a beginner with C++. The std::list already provides built in algorithms and iterator support providing many useful features and I doubt that you would want to redo all of that.
http://cplusplus.com/reference/stl/list/