i want to make sure that my linked list work

Question

Is this a right way to do a linked list ? I am having a problem in a big school project and now i want to make sure that this is true.

void addnode(int a){
    struct house* tmp = houses[i].next;
    while (tmp != NULL) {
        tmp = tmp->next;
    }
    tmp = (struct house*)malloc(sizeof(struct house));
    tmp->id=a;
    tmp->next=NULL;
}

i figured out that the error can be in other parts of the code. Now i will share the parts i suspect i hope you can help me. houses[i] is an array of linked lists. if houses[i].id==-1 it is empty

struct house get_house_byid(int id) {
    for (int i = 0; i < 1000; i++) {
        if (houses[i].id != -1) {
            if (houses[i].id == id) {
                return houses[i];
            }
            if (houses[i].next != NULL) {
                struct house* tmp = houses[i].next;
                while (tmp != NULL) {
                    if (tmp->id == id) {
                        return *tmp;
                    }
                    tmp = tmp->next;
                }
            }
        }
    }
    struct house housep;
    housep.id = -1;
    return housep;//if it cant find that id it returns housep
}

Your code looks like it will do what you want. Have you tested it to make sure that it works? What do you mean by "right way?" There's a way to write this using recursion; it's as "right" as your way is. — Robert Harvey, May 25 '21 at 15:05
Because it is a small portion of a larger code, where any other part can be broken — Eugene Sh., May 25 '21 at 15:13

Craig Estey · Accepted Answer · 2021-05-25T18:42:39.087

There may be other issues with your code that is not shown, but there are issues with addnode:

addnode does not set the head of the list (i.e. houses[i].next).
Thus, the newly added node is never connected to anything [and is a memory leak].
Ignoring the [obvious] typo/syntax error: void addnode{int a} instead of void addnode(int a).
The loop on tmp discards the pointer to the tail of the list. We need a separate variable (e.g. prev).
Note that i is global. That's fine, but the function would be cleaner if i was an argument to addnode instead.
Don't cast the return of malloc: Do I cast the result of malloc?

Here's is some refactored code. It is annotated:

void
addnode(int i,int a)
{
    struct house *tmp;
    struct house *prev;

    // find the tail of the list
    prev = NULL;
    for (tmp = houses[i].next;  tmp != NULL;  tmp = tmp->next)
        prev = tmp;

    // allocate the new node
    tmp = malloc(sizeof(*tmp));
    tmp->id = a;
    tmp->next = NULL;

    // append to the tail of the [non-empty] list
    if (prev != NULL)
        prev->next = tmp;

    // add to front of the empty list
    else
        houses[i].next = tmp;
}

You're welcome. Issues with one's first linked list implementation are a very common question here on SO, so you're not alone. Happy Programming! — Craig Estey, May 25 '21 at 18:39

i want to make sure that my linked list work

1 Answers1