3 replies to this topic

[meetjatin88]

[source lang="cpp"]#include <stdio.h>#include <ctype.h>#include <stdlib.h>#include<conio.h>#include<iostream>struct list{ int data; struct list *next;}*L;//function declaration:void create(int data,struct list **head);void print(struct list **head);int main(){ //list *L; int data =10; for(int i=0;i<3;i++) { //struct list *pList = NULL; create(data++,&L); print(&L); getch(); }}void create(int data,list **head){ list *pNew = new list; if (*head==NULL) { (*head)->next=NULL; (*head)->data=data; //(**head).data=info; //pNew->data =data; //pNew->next=*head; } else { pNew =*head; while(pNew!=NULL) { //*head=(**head).next; pNew=pNew->next; } list *temp =new list; temp->data=data; temp->next=NULL; pNew->next =temp; //(**head).next=(**head).next; } //return 0;}void print(list **head){ list *temp1 = new list; temp1 =*head; while( temp1!=NULL ){std::cout<< temp1->data<<" ";// show the data in the linked listtemp1 = temp1->next; // tranfer the address of 'temp1->next' to 'temp1'}}[/source]

Hello Guys,
I am a bit rusty on pointers,i actually want to implement linked list ,but my compiler is giving error.
i am getting run time error at line
(head)->next=NULL;
(head)->data=data;
Would be great if someone can review the short snippet code and point my mistake.
Any other suggestions are always welcome.
Please note that i have also tried using (**head).data=data; but then also i am still getting runtime error .
i am not sure how to proceeed next.

[Bacterius]

The code as presented will not compile, L has not been declared in main(). If your intention was the "list *L" line. then the reason it crashes is because you have not allocated space for the list which L should point to... you need a malloc (and a free) somewhere.

This is really C code with an cout thrown it, btw, not really C++.

Never mind.

Edited by Bacterius, 18 November 2012 - 11:10 PM.

[Álvaro]

L is declared globally, but in an unusual place (another symptom of old-C-programmer syndrome).

If your compiler is giving you an error, it would be good for you to post it. It might not mean anything to you, but chances are it does mean something to us, and we can help you understand it.

The section where you check if *head is NULL and then you proceed to fill it up with data should look like an obvious mistake to you.

I can only get this type of code right if I draw a bunch of boxes and arrows on a piece of paper, so I can keep track of what's pointing where in each operation.

[rip-off]

Please note that i have also tried using (**head).data=data; but then also i am still getting runtime error .

foo->bar is equivalent to (*foo).bar, so this change cannot affect the behaviour.

Álvaro is correct, the problem is that you are dereferencing a NULL pointer. Even when you fix that, there is another problem. Your create function leaks a node when the list is non-empty.

A simpler way to write it is to split the actions into two steps, the first being to create and populate the new node, and the second to add the node to the list.

void create(int data, list **head)
{
    list *node = new list;
    node->next = NULL;
    node->data = data;
   
    if(*head == NULL)
    {
	    *head = node;
    }
    else
    {
	    node *end = *head;
	    while(end->next != NULL)
	    {
		    end = end->next;
	    }
	    end->next = node;
    }
}

For a singly linked list, if the order of the list is irrelevant, it is faster to add the new element as the front of the list than the end.

You also leak memory in your print function. There is no need to allocate a new node, simply initialise the temporary node with the head:

void print(list **head)
{
    list *temp1 = *head;
    while(temp1 != NULL)
    {
	    std::cout << temp1->data << " ";
	    temp1 = temp1->next;
    }
}

Your print function would be simplified if it accepted a pointer as its parameter, rather than a pointer to a pointer:

void print(list *head)
{
    list *node = head;
    while(node != NULL)
    {
	    std::cout << node->data << " ";
	    node = node->next;
    }
}

There is no need for a pointer to a pointer because you do not need to modify the callee's pointer in this case.

Some of your headers are C++ specific, so let us write the code in a C++ style:

#include <cstdio>
#include <cctype>
#include <cstdlib>
#include <iostream>
#include <conio.h>

struct list
{
    int data;
    list *next;
};

void create(list *&head, int data)
{
    list *node = new list;
    node->data = data;
    node->next = null;
    if(head == NULL)
    {
	    head = node;
    }
    else
    {
	    node *end = head;
	    while(end->next != NULL)
	    {
		    end = end->next;
	    }
	    end->next = node;
    }
}
void print(list *head)
{
    list *node = head;
    while(node != NULL)
    {
	    std::cout << node->data << " ";
	    node = node->next;
    }
}

int main()
{
    list *L = NULL;
    int data = 10;
    for(int i = 0 ; i < 3 ; i++)
    {
	    create(L, data++);
	    print(L);
	    getch();
    }
}

The changes were:

• Change the standard headers includes to the C++ style (<cfoo> rather than <foo.h>
• Drop the "struct" prefix from declarations
• Drop the declaration of a global as part of the structure declaration*
• Using a reference rather than a pointer to implement "write back"

Special note on the global, it is unnecessary so as part of the changes I've made it a local variable. In particular that because it is a local variable it must be initialised to NULL, whereas previously as a global it defaulted to being initialised as NULL.

A final note, modifying the caller's variable is more surprising than the simpler approach of using the function's return value. Here is another alternative:

// ...

list *append(list *head, int data)
{
    list *node = new list;
    node->data = data;
    node->next = null;
    if(head == NULL)
    {
	    return node;
    }
    else
    {
	    node *end = head;
	    while(end->next != NULL)
	    {
		    end = end->next;
	    }
	    end->next = node;
	    return head;
    }
}

int main()
{
    list *L = NULL;
    int data = 10;
    for(int i = 0 ; i < 3 ; i++)
    {
	    L = append(L, data++);
	    print(L);
	    getch();
    }
}