CODEWITHSUNDEEP
cdata-structureslinked-list
user2538255
user2538255

Reputation: 313

code not printing linked list C

I am writing C code to implement linked list.But while printing the contents of the list it prints the values taken for last node only.I have been debugging for long.Please help.

#include <stdio.h>
#include <malloc.h>

struct list {
    char *name;
    char *type;
    int occurance;
    struct list *prev;
    struct list *link;
};

struct list *start=NULL,*ptr,*newnode;

void main() {
    int choice = 0;
    char name1[10], type1[10];
    int occ;

    do {
        printf("Enter name:");
        scanf("%s", name1);
        printf("Enter type:");
        scanf("%s", type1);
        printf("Enter occurance:");
        scanf("%d", &occ);
        newnode = (struct list *)malloc(sizeof(struct list));
        newnode->link = NULL;
        newnode->prev = NULL;
        newnode->name = name1;
        newnode->type = type1;
        newnode->occurance = occ;
        if(newnode == NULL) {
            printf("Memory could not be allocated!");
            // exit(0);
        }
        if(start == NULL) {
            start = newnode;
            ptr = start;
            printf("start is: %s", start->name);
        }
        else if(start->link == NULL) {
            start->link = newnode;
            newnode->prev = start;
            ptr = newnode;
        }
        else {
            ptr->link = newnode;
            newnode->prev = ptr;
            ptr = ptr->link;
        }
        printf("Enter 1 to continue: ");
        scanf("%d", &choice);
    } while(choice == 1);

    // display
    ptr = start;
    while(ptr != NULL) {
        printf("%s ", ptr->name);
        printf("%s ", ptr->type);
        printf("%d \n", ptr->occurance);
        ptr = ptr->link;
    }
}

I have tried making start and newnode local variables as well but it doesn't work.

Upvotes: 1

Views: 131

Answers (2)

ajay
ajay

Reputation: 9680

There are several things to improve in your code so let's take it one by one. First, the behaviour you are seeing is because the structure members name and type of all nodes in your linked list point to the same memory location, i.e, to name1 and type1 respectively. 

struct list {
    char *name;
    char *type;
    // other members
};

Here, name is a pointer to a character and so is type. In the do while loop in main, you read name and type of each node into the arrays name1 and type1 respectively in each iteration. Therefore, the previous values of name1 and type1 are overwritten and hence lost. Now you are assigning name1 and type1 to respective members of each newly created node.

// after allocating memory for a new node

newnode->name = name1;  // name1 evaluates to a pointer to name1[0]
newnode->type = type1;  // type1 evaluates to a pointer to type1[0]

Therefore name member of all nodes are pointing to name1 which only retains the last string read, and similarly type member of all nodes point to type1 which also has the last string read.

Now that we have discussed the behaviour of your program, what's the solution? You keep name and type of each node from getting overwritten by the next values entered. You do this by allocating memory for them every time you read them for a new node.

newnode->name = strdup(name1); // make a copy of name1 before it's overwritten
newnode->type = strdup(type1);  // make a copy of type1 before it's overwritten

strdup allocates enough memory to copy the string passed to it. It essentially does the same thing as malloc and strcpy. Read more about it here.

Now let's come to other things starting from the top of your program.

  • malloc.h is deprecated and not standard. Use stdlib.h instead. More here.
  • The C standard lays down two signatures for the main function: int main(void); and the other is int main(int argc, char *argv[]); More here.
  • While reading strings using scanf, you should guard against buffer overrun. If the user enters name and type which are longer than 9 characters, it will lead to undefined behaviour and even crash. You should replace scanf("%s", name1) with scanf("%9s", name1). The 9 in the format string means that scanf will read at most 9 non-whitespace characters. One character space is left for the terminating null byte which it adds automatically.
  • Check the return value of malloc for NULL immediately after the call. You program as such will simply crash if malloc fails to allocate memory.
  • You don't need the else if block in your do while loop. It will never be entered except in the first iteration of the loop when it makes start->previous = start which is not what you want. start->previous should always be NULL.

Upvotes: 0

bagage
bagage

Reputation: 1114

Your cannot use equal operator to assign a char *. Instead you must use function strcpy.

Do not do this:

newnode->name=name1;
newnode->type=type1;

But do this:

strcpy(newnode->name, name1);
strcpy(newnode->type, type1);

Currently your whole char* are pointing to the same memory block.

Edit: Since you are using pointers, you must allocate memory before copying value from one pointer to another (or you will encounter segfaults). So you also need to allocate your node's name and type memories with malloc:

//allocate memory of node's name attribute with the same amount as name1 (+1 for null terminating character)
newnode->name = malloc(sizeof(char) * (strlen(name1)+1));
newnode->type= malloc(sizeof(char) * (strlen(type1)+1));

Upvotes: 1

Related Questions