1

I'm a beginner in developing, so my sensei gave me a task to complete in which I need to enter a couple of strings in linked lists and after I enter print, they need to be printed in the correct order, from the first to last.

Here is what I got: 

#include <stdio.h> 
#include <stdlib.h> 
#include <string.h>

    typedef struct Node {
        char data;
        struct Node *next;
    }node;

char createlist(node *pointer, char data[100]) {
    while (pointer->next != NULL) {
        pointer = pointer->next;
    }

    pointer->next = (node*) malloc(sizeof(node));
    pointer = pointer-> next;
    pointer->data = *data;
    pointer->next = NULL;
}

int main() {
    node *first, *temp;
    first = (node*) malloc(sizeof(node));
    temp = first;
    temp->next = NULL;

    printf("Enter the lines\n");
    while (1) {
        char data[100];
        gets(data);
        createlist(first, data);
        if (strcmp(data, "print") == 0)
            printf("%s\n", first->data);
        else if (strcmp(data, "quit") == 0)
            return (0);

    };

}

When I run it I get: Enter the lines: asdfasdf print (null)

Any help would be appreciated since this is my first time using linked lists.

Ludwig
  • 432
  • 6
  • 17
Mirakurun
  • 4,859
  • 5
  • 16
  • 32

2 Answers2

3
  • You should format your code properly.
  • first->data is allocated via malloc() and isn't initialized, so using its value invokes undefined behavior.
  • In order not to deal the first element specially, you should use pointer to pointer to have createlist() modify first.
  • Since createlist() won't return anything, type of its return value should be void.
  • I guess you wanted to copy the strings instead of assigning the first character of each strings.
  • To print all of what you entered, code to do so have to be written.
  • You shouldn't use gets(), which has unavoidable risk of buffer overrun.
  • You should free() whatever you allocated via malloc().

improved code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

typedef struct Node
{
    char *data;
    struct Node *next;
} node;

void createlist(node **pointer, char data[100])
{
    while (*pointer != NULL)
    {
        pointer = &(*pointer)->next;
    }

    *pointer = malloc(sizeof(node));
    if (*pointer == NULL)
    {
        perror("malloc 1");
        exit(1);
    }
    (*pointer)->data = malloc(strlen(data) + 1);
    if ((*pointer)->data == NULL)
    {
        perror("malloc 2");
        exit(1);
    }
    strcpy((*pointer)->data, data);
    (*pointer)->next = NULL;
}

int main(void)
{
    node *first = NULL;

    printf("Enter the lines\n");
    while (1)
    { 
        char data[100], *lf;
        if (fgets(data, sizeof(data), stdin) == NULL) strcpy(data, "quit");
        if ((lf = strchr(data, '\n')) != NULL) *lf = '\0'; /* remove newline character */
        createlist(&first, data);
        if (strcmp(data, "print") == 0)
        {
            node *elem = first;
            while (elem != NULL)
            {
                printf("%s\n", elem -> data);
                elem = elem->next;
            }
        }
        else if (strcmp(data, "quit") == 0)
        {
            while (first != NULL)
            {
                node *next = first->next;
                free(first->data);
                free(first);
                first = next;
            }
            return(0);
        }

    }

}
MikeCAT
  • 73,922
  • 11
  • 45
  • 70
  • Thank you for trying to help. I get these errors: main.cpp:18:35: error: invalid conversion from ‘void*’ to ‘node* {aka Node*}’ [-fpermissive] *pointer = malloc(sizeof(node)); main.cpp:24:47: error: invalid conversion from ‘void*’ to ‘char*’ [-fpermissive] (*pointer)->data = malloc(strlen(data) + 1); – Mirakurun Mar 02 '16 at 15:44
  • @Mirakurun Use C compiler, not C++ compiler. This question is marked as C in both the title and the tag, and the code is C. Why on earth you did compile this code as C++? – MikeCAT Mar 02 '16 at 15:45
  • Yeah, that was my bad, didn't even see that my extension is cpp. I just configured C on Kdevelop and it works flawless. Thank you again. – Mirakurun Mar 02 '16 at 16:21
0

Inside createlist(), you are iterating to the end of the list. There, you are adding a new node and setting a new text entered. By doing so, you are missing that you have already a first node. Because you are iterating to the end in every call of createlist(), you are jumping over your first node every time, so it remains without text and delivers NULL.

In order not to jump over the first initial node, you could alter createlist() like this: 

char createlist(node *pointer, char data[100])
{
   while (pointer->data != NULL && pointer->next != NULL)
   {
     pointer = pointer->next;
   }
   ...
   ... 
}

Or you could create the first node not initially, but only after the first line of text was entered.

edit: Here are two additional style hints:

  • What happens if somebody enters 120 characters? The text will outrun your char[100] array and will fill RAM that is used otherwise. This is a buffer overflow. You could try to grab only the first 100 chars, get the substring. Alternatively, use the length argument of fgets()

  • Create a constant for 100, like #define MAX_BUFFER_LENGTH 100, and use it every time.

Community
  • 1
  • 1
peter_the_oak
  • 3,529
  • 3
  • 23
  • 37