Felet med min länkade lista är att efter det att jag har satt in en ny nod i den så verkar det som det föregående objektets itsNext-nod inte pekar på den nya noden. Tacksam för hjälp, koden ser du nedan:


#include <iostream.h>

class Node
{
public:
Node();
Node(int num);
~Node();

Node* GetNext() const { return itsNext; }
int GetNumber() { return number; }
void Insert(Node *);
void SetNext(Node * node) { itsNext = node; }
private:
Node * itsNext;
int number;
};


Node::Node():
itsNext(0)
{}


Node::Node(int num):
number(num), itsNext(0)
{}


Node::~Node()
{
delete itsNext;
itsNext = 0;
}


void Node::Insert(Node* newNode)
{
itsNext = newNode;
}


int main()
{
Node* pCurrent = new Node(1);
Node* pNode = new Node;
Node* pHead = new Node;

pCurrent->Insert(pNode);
pHead = pCurrent;
pCurrent = new Node(2);
pCurrent->Insert(pNode);

for (int i=0; i<10; i++)
{
cout << "one: " << pCurrent->GetNumber() << endl;
pCurrent->GetNext();
}

delete pHead;

return 0;
}

1. Du använder namnet itsNext på pekaren till nästa. Ovanligt namn. Spelar ju egentligen ingen roll, men bör ändå vara pNext eller pnNext.

2. Dina två funktioner Insert och SetNext är identiska. Ta bort ena så får du bättre struktur på koden. Eftersom du har en enkellänkad lista har du inte möjlighet att sätta in en nod före en utpekad.

3. Du använder i loopen pCurrent->GetNext(); vilket skulle returnera en pekare till nästa. Detta betyder ju dock inte att du får fram nästa.
Du bör skriva loopen så här:

for(Node *pOne=pHead; pOne; pOne=pOne->GetNext())
cout << "one: " << pCurrent->GetNumber() << endl;

4. Ett sätt att förenkla hanteringen väsentligt är att ha en List-class.
Denna skall sedan hantera hela listan. (Innehålla pHead, osv.)

/Niklas Jansson

Jag har gjort som du skrev men listan funkar ändå inte. Så här ser den ut nu:

#include <iostream.h>

class Node
{
public:
Node();
Node(int num);
~Node();

Node* GetNext() const { return nextNode; }
int GetNumber() { return number; }
void Insert(Node* node) { nextNode = node; }
private:
Node * nextNode;
int number;
};


Node::Node():
nextNode(0)
{}


Node::Node(int num):
number(num), nextNode(0)
{}


Node::~Node()
{
delete nextNode;
nextNode = 0;
}


int main()
{
Node* pCurrent = new Node(1);
Node* pNode = new Node;
Node* pHead = new Node;

pCurrent->Insert(pNode);
pHead = pCurrent;
pCurrent = new Node(2);
pCurrent->Insert(pNode);

pCurrent = pHead;
while(pCurrent)
{
cout << "number: " << pCurrent->GetNumber() << endl;
if (pCurrent->GetNext)
pCurrent = pCurrent->GetNext();
}

delete pHead;

return 0;
}

Jag har lite bråttom men din kod i Node verkar vara rätt.

Något sånt här skulle jag ha gjort i Main:

Node *pHead;
Node *pTemp;
Node *pCurrent;

pTemp = new Node(1);
pCurrent = pHead = pTemp;

pTemp = new Node(2);
pCurrent.Insert (pTemp);
pCurrent=pCurrent->GetNext();

pTemp = new Node(3);
pCurrent.Insert (pTemp);
pCurrent=pCurrent->GetNext();

pTemp = new Node(4);
pCurrent.Insert (pTemp);
pCurrent=pCurrent->GetNext();

----

Du bör inte ha namnet Insert. Du borde ha SetNext och Insert
Insert skulle du istället ha på en funktion:
class Node
...
Insert(Node *NewNode)
{
NewNode->SetNext(nextNode);
nextNode=NewNode;
}

(Observera att detta bör användas med försiktighet. Det skulle finnas lite kontroller.)

/Niklas Jansson

Nu fick jag äntligen till listan efter lite ändringar och krånglande. Tack för hjälpen!

Lite förbättringar som brukar vara bra:

1. Ge möjlighet att skicka med adressen till nästa vid konstruktionen.
Node::Node(int Num, *Next)

2. Använd en fristående klass, (Linked)List som hanterar listan. Du kan fortfarande låta Node vara allmän, för att ha en iterator, men gör en

class List
{
//Funktioner
...
private:
Node *pHead;
long lLength;
}

Det är lämpligt eftersom du kan cacha längden lätt.


/Niklas Jansson

Hur skulle jag lämpligtvis kunna göra för att plocka bort noder?
Jag har försökt på ett antal sätt utan att lyckas.

Du bör ha en funktion - ta bort _efter_ nod, i och med att du bara har en enkellänkad lista.
Då gör du så här:
1. Spara pekaren till nästa nod (dvs. den du skall ta bort).
2. Byt ut pekaren som förut pekade på noden som skall tas bort till noden efter.
3. Ta bort noden.

void Node::RemoveNext ()
{
Node *pTemp=nextNode;
nextNode=nextNode->GetNext();
delete pTemp;
}

Annars få du köra med dubbellänkad lista, dvs. en pekare till föregående nod.

/Niklas Jansson

Jag testade att göra som du skrev, men det funkade inte så jag gjorde listan dubbellänkad och skapade en egen funktion för att ta bort noder. Problemet med DeleteNode-funktionen är att programmet kraschar när man använder sig av funktionen, men om man skippar den sista raden, där det står "fel?" så funkar det, men då kan man bara gå igenom listan framifrån. Så här ser funktionen ut:

void Node::DeleteNode(Node *node)
{
if (node)
{
Node *pPrevNode = node->GetPrev();
Node *pNextNode = node->GetNext();

delete node;
node = NULL;

pPrevNode->nextNode = pNextNode;
pNextNode->prevNode = pPrevNode; // fel?
}
}

/ Peter Larsson

Antingen får du ha en medlemsfunktion

void Node::Remove();

eller så får du ha en friend

void RemoveNode (Node *pNode);

Du kan ha båda samtidigt men det brukar krångla till för andra som vill använda koden.

-Förresten. Jag är osäker på om det funkar med en medlemsfunktion. Du kan försöka, men lägg inte ner mycket tid på den.

Ett problem är att du måste kolla så att du inte tar bort första eller sista i en lista. Då får du nämligen problem. Tar du bort första bör du ge möjlighet att ändra head, eller göra en cirkulär lista. (dvs sista nodens next pekar mot första noden och vice versa).

Välj först hur du skall göra så återkommer jag med svar.

/Niklas Jansson

För att ge möjlighet att ändra head (och lämpligtvis tail också) gör du något sånt här (om du inte använder en cirkulär lista):

void RemoveNode (Node *pNode, Node *&pHead, Node *&pTail)
{
if (pNode=pHead)
{
pHead=pHead->GetNext();
pHead->SetPrevious(0);
delete pNode;
}
else if (pNode=pTail)
{
pTail=pTail->GetPrevious();
pTail->SetNext(0);
delete pNode;
}
else if(pNode)
{
pTemp=pNode;
pNode->GetPrevious()->SetNext(pNode->GetNext());
pNode->GetNext()->SetPrevious(pNode->GetPrevious());
delete pNode;
}
}

---

Sen tänkte jag på en annan sak; du kallar pekarna nextNode och prevNode, va? Det är lite dumt, följer inte de vanligaste konventionerna.
Kalla dem pNext och pPrev istället. Att det är pekare till noder framgår ju av deklarationen.

Som du ser av koden ovan så blir det mycket lättare om du använder en klass för själva listan, och en för noderna. Gör listan till en friend-class av Node, och låt bara yttre användare få använda GetNext, GetPrevious och GetValue (eller vad du nu kallar den). Du kan även ta med SetValue.

/Niklas Jansson