1
\$\begingroup\$

Nodes will be for simply linked lists, circular lists, maybe later on extending them to work with graphs. I'm trying to be as defensive as possible, will this leak memory? It runs perfectly I'm just concerned about coding style and best practices. Especially regarding the allocation I'm not 100% convinced of my solution. Is it okay to allocate memory for the next component inside the constructor?

// I would have preferred to declare the typedef together with the struct, 
// but this doesn't work.
typedef struct node Node;   


struct node {
    int value;
    Node *next;
};


Node *create_node(int value) {

    Node *result = malloc(sizeof(Node));
    // How necessary are these checks? Under which circumstances 
    // could this return NULL?
    if (result == NULL) {           
        return NULL;
    }

    result->next = malloc(sizeof(Node));
    if (result->next == NULL) {
        free(result);
        return NULL;
    }

    result->value = value;

    return result;
}


void destroy_node(Node *node) {
    if (node != NULL) {
        //This seems necessary. Or is it not? Could I delete the parent 
        // node and keep the children alive?
        destroy_node(node->next);   
        free(node);
    }
}

int main() {

    Node *first = create_node(42);
    printf("%d\n", first->value);

    Node *second = create_node(45);
    first->next = second;
    printf("%d\n", first->next->value);

    destroy_node(first);

    return 0;
}
\$\endgroup\$
2
  • \$\begingroup\$ For Code Review, we expect you to be reasonably confident that the code works as intended. Please tell us more about what kind(s) of data structure you intend to create with these nodes. Include some code to demonstrate sample usage. \$\endgroup\$ Oct 17, 2016 at 19:27
  • \$\begingroup\$ Added a few lines about the intention plus sample code to test functionality. \$\endgroup\$
    – AdHominem
    Oct 17, 2016 at 19:33

1 Answer 1

1
\$\begingroup\$

It's very misleading that destroy_node (singular) actually recursively frees all nodes in the chain.

It's very weird that create_node allocates two nodes rather than one. In fact, you do have a memory leak in your main():

first->next = second;

There, you have just overwritten the 42 node's next pointer without having freed it.


To answer this question…

// How necessary are these checks? Under which circumstances 
// could this return NULL?
if (result == NULL) {           
    return NULL;
}

If malloc() fails (because no more memory is available), it will return NULL. Yes, it is good practice to check whether malloc() failed. (In practice, if you are so low on memory that you can't even allocate a single linked-list node, it's very difficult to recover gracefully. Your program is going to die, and there's not much you can do about it.)

\$\endgroup\$
2
  • \$\begingroup\$ Thanks. Would it be better to just assign NULL to the internal Node? I thought I needed to malloc both based on first answer here: stackoverflow.com/questions/14768230/… \$\endgroup\$
    – AdHominem
    Oct 17, 2016 at 19:49
  • \$\begingroup\$ Yes, I would expect create_node() to set result->next = NULL;. \$\endgroup\$ Oct 17, 2016 at 19:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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