critique my code: attempting to make a linked list

[rich wind]

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


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


Node *create_node(int value)
{
    Node *node = malloc(sizeof(Node));
    node->value = value;
    node->next = NULL;

    return node;
}


void print_node_value(Node *head)
{
    while(head != NULL)
    {
        printf("%d\n", head->value);
        head = head->next;
    }
}


void add_value(Node *head, int value)
{
    while(head->next != NULL)
    {
        head = head->next;
    }

    head->next = malloc(sizeof(Node));
    head->next->value = value;
    head->next->next = NULL;
}


Node *reverse_list(Node *head)
{
    Node *prev = NULL;
    Node *curr = head;
    Node *next = malloc(sizeof(Node));

    while(curr != NULL)
    {
        next = curr->next;
        curr->next = prev;

        prev = curr;
        curr = next;
    }

    return prev;
}


int main()
{
    Node *head = create_node(10);

    add_value(head, 20);
    add_value(head, 30);
    add_value(head, 40);
    add_value(head, 50);
    add_value(head, 60);
    add_value(head, 70);

    print_node_value(head);

    printf("\nNOW REVERSE\n\n");

    head = reverse_list(head);

    print_node_value(head);

    return 0;
}

[next sentinel]

When your question is answered use !solved to mark the question as resolved.

Remember to ask specific questions, provide necessary details, and reduce your question to its simplest form. For tips on how to ask a good question use !howto ask.

[rich wind]

any suggestions for the way i implemented this?

[lean plank]

Why don't you use create_node in add_value?

[pulsar osprey]

I feel like add_value and create_node is kind of redundant

[vagrant field]

make stuff const if possible, add_value is currently O(n)

[rich wind]

why?

[rich wind]

i never thought about it lol

[lean plank]

Why do you even allocate memory for next in reverse_list?
That is a) unnecessary and b) creates a memory leak

[pulsar osprey]

It's hard to articulate. Maybe it just struck me as odd that there are 2 functions that take a value and allocate a node?

Not the worst thing in the world, so whatever

[pulsar osprey]

Oh good catch, didn't see that

[rich wind]

i thought it would throw an error if i didnt

[lean plank]

You also should implement a destroy_list/destory_node function

[rich wind]

how does it create a memory leak?

[pulsar osprey]

As long as it is assigned a value before it's read, (in this case next = curr->next) you are good

[lean plank]

Because you either set next to curr->next or just return prev. In either case the newly malloced memory is a leak

[pulsar osprey]

All memory allocated, needs to be freed. You allocate data for next, then immediately discard the pointer without freeing

[rich wind]

well im mallocing memory in the other functions

WITHOUT freeing

is that a memory leak too?

[pulsar osprey]

Yes and no.
That memory is still being used, so you still want it to be allocated.
However, the moment you stop using it without freeing it, that would be a memory leak

[lean plank]

That is why i said you need a destroy_list/destroy_node function to free the stuff at the end of your main

[rich wind]

ohhh

[vagrant field]

I feel like the name print_node_value implies that it prints a single node, not the entire list.
And add_value is too unspecific imo, it could be called append_node or push_back or something. Inserting at the head would be O(1) instead of O(n)

[rich wind]

true

okay question

the destroy list function, would i just walk the list and set the nodes to NULL then free them?

[lean plank]

Also, you should check every malloc return, whether NULL is returned

That is an error

[rich wind]

good idea

[lean plank]

No just free them

If you set it to NULL you can't free afterwards

[rich wind]

i see

[pulsar osprey]

So you'll have to free the list in reverse order, as if you free the first node, you can't read that node to find the next

[rich wind]

alright then

good critiques

thanks guys

[lean plank]

👍

[vagrant field]

I think you can also free the list in the usual order, as long as you store node->next in a variable before freeing node

[lean plank]

Yes

[pulsar osprey]

You code is overall really good for your current level.
Most issues are just tips about the next topic to learn

[lean plank]

Funky idea, never thought of it 😄

Yes, Zero is right, over all solid thingy^^

[rich wind]

"Most issues are just tips about the next topic to learn" what do you mean?

[lean plank]

Your issues are just stuff you should learn in more detail^^

[pulsar osprey]

Basically I'm imagining an example class where lesson 10 is malloc and lesson 12 is free, you are currently in the middle.

[rich wind]

alright, thanks again <3

[next sentinel]

@rich wind Has your question been resolved? If so, type !solved :)

[rich wind]

!solved

[next sentinel]

Thank you and let us know if you have any more questions!

This thread is now set to auto-hide after an hour of inactivity

[rich wind]

one last thing

@lean plank sorry for the ping

[lean plank]

Sure^^

[rich wind]

void destroy_list(Node *head)
{
    Node *temp;

    while(head != NULL)
    {
        temp = head;
        head = head->next;
        free(temp);
    }
}```

does this work? sorry im a noob

[lean plank]

Yes

Works

[rich wind]

awesomesauce