patterncMinor
Sparse linked list insert function
Viewed 0 times
insertfunctionlinkedlistsparse
Problem
I wrote this sparse linked list insert function, and there are nine
```
/* NOT BUGS, TESTED AND WORKS CORRECTLY
* insert an element into a list
* list is ordered using pos
* if position pos is already occupied, the value of the node
* should be updated with val
* if val=0, then the element should be deleted
* return 0 if operation is succesfull
1 if malloc failed /
int insert_element(ElementNode_handle *list_handle, int pos, int data)
{
/Record the head/
ElementNode current = list_handle;
/ If data is 0, stop /
if (data == 0)
{
return 1;
}
/ If list is empty, crate new list/
if (current == NULL)
{
/ Create new list /
ElementNode *new_node = make_node(pos, data);
/ Malloc fail check/
if (new_node == NULL)
{
return 0;
}
new_node->data = data;
new_node->pos = pos;
*list_handle = new_node;
return 1;
}
/ If head pos == pos, replace and done/
if (current->pos == pos)
{
current->data = data;
return 1;
}
else if (current->pos > pos)
{
/ Create new node between current and next /
ElementNode *new_node = make_node(pos, data);
if (new_node == NULL)
{
return 0;
}
new_node->next = current;
*list_handle = new_node;
return 1;
}
/Walk the list, until next hits the end/
while (current->next != NULL)
{
if (current->next->pos == pos)
{ / If next pos equals post, replace, done/
current->next->data = data;
return 1;
}
else if (current->next->pos > pos)
{
/ Create new node between current and next /
ElementNode *new_node = make_node(pos, data);
if (new_node == NULL)
{
return 0;
}
ElementNode *next = current->next;
current->next = new_node;
new_node->next = next;
return 1;
}
return statements. Is this a code smell? Is this badly implemented? I fear that it's hard to read, or hard to maintain.```
/* NOT BUGS, TESTED AND WORKS CORRECTLY
* insert an element into a list
* list is ordered using pos
* if position pos is already occupied, the value of the node
* should be updated with val
* if val=0, then the element should be deleted
* return 0 if operation is succesfull
1 if malloc failed /
int insert_element(ElementNode_handle *list_handle, int pos, int data)
{
/Record the head/
ElementNode current = list_handle;
/ If data is 0, stop /
if (data == 0)
{
return 1;
}
/ If list is empty, crate new list/
if (current == NULL)
{
/ Create new list /
ElementNode *new_node = make_node(pos, data);
/ Malloc fail check/
if (new_node == NULL)
{
return 0;
}
new_node->data = data;
new_node->pos = pos;
*list_handle = new_node;
return 1;
}
/ If head pos == pos, replace and done/
if (current->pos == pos)
{
current->data = data;
return 1;
}
else if (current->pos > pos)
{
/ Create new node between current and next /
ElementNode *new_node = make_node(pos, data);
if (new_node == NULL)
{
return 0;
}
new_node->next = current;
*list_handle = new_node;
return 1;
}
/Walk the list, until next hits the end/
while (current->next != NULL)
{
if (current->next->pos == pos)
{ / If next pos equals post, replace, done/
current->next->data = data;
return 1;
}
else if (current->next->pos > pos)
{
/ Create new node between current and next /
ElementNode *new_node = make_node(pos, data);
if (new_node == NULL)
{
return 0;
}
ElementNode *next = current->next;
current->next = new_node;
new_node->next = next;
return 1;
}
Solution
Do what you say
So if a
You
You
Keep it simple
Why do you need
You save an assignment and a variable declaration by just changing the order in which two lines happen.
* return 0 if operation is succesfull
* 1 if malloc failed */So if a
malloc fails, you are supposed to return 1. But when you do the actual check: ElementNode *new_node = make_node(pos, data);
/* Malloc fail check*/
if (new_node == NULL)
{
return 0;
}You
return 0. You
return 1 on success -- the exact opposite of the comment. You should either update the comment to match the code or change the code to match the comment. Or remove the comment--if you have to read the code anyway, why bother? Keep it simple
ElementNode *next = current->next;
current->next = new_node;
new_node->next = next;Why do you need
next? Consider new_node->next = current->next;
current->next = new_node;You save an assignment and a variable declaration by just changing the order in which two lines happen.
Code Snippets
* return 0 if operation is succesfull
* 1 if malloc failed */ElementNode *new_node = make_node(pos, data);
/* Malloc fail check*/
if (new_node == NULL)
{
return 0;
}ElementNode *next = current->next;
current->next = new_node;
new_node->next = next;new_node->next = current->next;
current->next = new_node;Context
StackExchange Code Review Q#143595, answer score: 5
Revisions (0)
No revisions yet.