current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I am learning C from K.N. King and at ch-17, I covered linked list and wrote a code to add data in the ascending order in the linked list which is opposite to found in the book (In the book, it store in reverse order). Here's the code:

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

typedef struct NODE {
    int value;
    struct NODE *next;
} Node;

int main(void) {
    Node *root = NULL;
    Node *connector;
    int n;
    for(n = 1; n < 5; n++) {

        if(root == NULL) {
            connector = malloc(sizeof(Node));
            connector->value = n;
            connector->next = NULL;
            root = connector;
        }
        else {
            connector->next = malloc(sizeof(Node));
            connector->next->value = n;
            connector = connector->next;
            connector->next = NULL;
        }

    }

    while(root != NULL) {
        printf("%d\n", root->value);
        root = root->next;
    }

    return 0;

}

Can this code be improved. I know it's very basic and I just write for testing only. However I can make a function for it and loop is not necessary also. What I want to know is that, can I reduce the if-else to single if statement.

share|improve this question

1 Answer 1

up vote 2 down vote accepted

Well you can get rid of the if-else by avoiding a malloc of the root node like this:

Node root = {0, NULL};
Node *tail = &root;

for (int n = 1; n < 5; n++) {
    Node *node = malloc(sizeof(Node));
    node->value = n;
    node->next = NULL;
    tail->next = node;
    tail = node;
}

for (const Node *node = &root; node != NULL; node = node->next) {
    printf("%d\n", node->value);
}

This avoids the if-else, as you asked, but means you must be careful never to free the root node (as that will fail). Note that the printing loop changes accordingly.

Alternatively, you could extract the common code from the if-else and keep the root node as a pointer. This doesn't get rid of the if-else, but simplifies it for the reader.

Node *root = NULL;
Node *tail = NULL;

for (int n = 1; n < 5; n++) {
    Node *node = malloc(sizeof(Node));
    node->value = n;
    node->next = NULL;

    if (root == NULL) {
        root = node;
    } else {
        tail->next = node;
    }
    tail = node;
}

Note that in both cases I used the variable tail rather than connector, as it seems more descriptive, and a temporary variable node. I also defined the loop variables within the loop, which is preferable.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.