#include <stdio.h>
#include <stdlib.h>
typedef struct Node {
int val;
struct Node* next;
} node_t;
typedef struct List {
node_t* head;
} LinkedList;
void insert(int data, LinkedList* ll) {
node_t* newNode = (node_t*) malloc(sizeof(node_t));
newNode->next = NULL;
newNode->val = data;
if (ll->head == NULL) {
ll->head = newNode;
}
else if (ll->head != NULL)
{
node_t* current = newNode;
while (current->next != NULL) {
current = current->next;
}
current->next = newNode;
}
}
void display(LinkedList ll) {
node_t* temp = ll.head;
unsigned int ind = 1;
while (temp != NULL) {
printf("%d\t%d\n", ind, temp->val);
temp = temp->next;
ind++;
}
}
int main(void) {
LinkedList newList;
insert(5, &newList);
insert(8, &newList);
insert(7, &newList);
display(newList);
return 0;
}```
basically the problem here is that when I want to go on and display the newList, i only get the first element printed. i am having trouble finding the functionality error#incorrect linkedlist functionality
next arrow
silk blazeBOT
honest parrot
```C
#include <stdio.h>
#include <stdlib.h>
typedef struct Node {
int val;
...
;compile -Wall -Wextra -fsanitize=address,undefined
elfin joltBOT
honest parrot
```C
#include <stdio.h>
#include <stdlib.h>
typedef struct Node {
int val;
...
Found your error.
It's in this block of code:
else if (ll->head != NULL)
{
node_t* current = newNode;
while (current->next != NULL) {
current = current->next;
}
current->next = newNode;
}
- I would change that function's name to
appendrather thaninsert, becauseinsertimplies that you can put the new element at any place whileappendis clear that you add it to the end. - You can change the
else if (ll->head != NULL)to justelsebecause you already checked forll->head == NULLthere. - Instead of writing
if (ll->head != NULL)you should writeif (ll->head)and instead of writingif (ll->head == NULL)you should writeif (!ll->head)to improve readability. - Also in your
mainyou forgot to set thenewList.headfield toNULL. In this case you just got lucky it wasNULLby default, but there's no gurantee that it actually is. This means that right now your code has UB (undefined behaviour) and would crash if you didn't luck out on getting anewList.head = NULL. For this reason you need to either donewList.head = NULLor simply in the line where you allocate it you'd write this:LinkedList newList = { 0 };
Besides these details this is a pretty good implementation so far
I especially like that you have an extra LinkedList struct and not just operate on the Nodes directly
next arrow
- I would change that function's name to `append` rather than `insert`, because ...
thank you a lot for the suggestions and feedback
i really appreciate it
however
oh wait
silk blazeBOT
@next arrow Has your question been resolved? If so, type !solved :)
honest parrot
Btw, I didn't fix the ultimate flaw for you, nor did I describe how to fix it in the bullet points. I just gave you the part of the code responsible for the problem.
next arrow
ive been trying to solve this for the past 40 minutes but i just cant figure it out
can you give me a hint or something
honest parrot
ive been trying to solve this for the past 40 minutes but i just cant figure it ...
The error is in this line:
node_t* current = newNode;
yup, exactly
next arrow
else if (ll->head != NULL)
{
node_t* current = newNode;
while (current->next != NULL) {
current = current->next;
}
current->next = newNode;
}```
so im guessing basically since we are assigning current->next to the newNode, it would basically be the same a(if were inserting the second element) to newNode->next = newNode, which im not even sure what it does but by doing it with ll->head the thing is somehow done correctly meaning i dont completely understand why this ishonestly i'll go through this sometime later but i thank you a lot for your time
really cool stuff from you
!solved
silk blazeBOT
honest parrot
```c
else if (ll->head != NULL)
{
node_t* current = newNode;
while (cur...
Because the newNode was just before defined like so:
node_t* newNode = (node_t*) malloc(sizeof(node_t));
newNode->next = NULL;
newNode->val = data;
It is just a node that isn't connected to your linked list at all right now and as you can see newNode->next == NULL (because that's how you set it).
So you need to do
node_t *current = ll->head;
```because in the `while (current->next)` loop you want to iterate over your list (that begins at `ll->head`) until you've reached the last element in your list and after that element you then insert `newNode`.
If you write:
```c
node_t *current = newNode;
```then the `while (current->next)` won't ever iterate even once, because - as explained in the beginning - we just set `newNode->next` to be `NULL`.
Also then the line `current->next = newNode` breaks, because what you essentially wrote then is `newNode->next = newNode` where `newNode` is then it's own successor (and therefore also it's own predecessor), i.e. you build an infinite list containing only a single element.
And to top it all off, since you never inserted it into your linked list `ll`, the temporary variable `newNode` will just cease to exist after the `insert` function ended, and to really top it off this also causes a memory leak because you now have a `malloc` without a `free` (and in fact you can't ever `free` that memory again cause you won't have an idea where it was allocated).