HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Sparse linked list insert function

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
insertfunctionlinkedlistsparse

Problem

I wrote this sparse linked list insert function, and there are nine 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

* 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.