Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

My first bug bear is identifiers should not begin with an underscore:

typedef struct _node {
typedef struct _llist {

See: what-are-the-rules-about-using-an-underscore-in-a-c-identifier what-are-the-rules-about-using-an-underscore-in-a-c-identifier

Rather than having any output parameters I think it is easier to see behavior by returning results:

node* init_node(int value )
{
 node* new_node = (node*)malloc( sizeof(node) );
 new_node->value = value;
 new_node->next = NULL;
 return new_node;
}

/* not sure what I should do here. should I hide the 'error' or force an assert to fail if the caller tries to insert into an empty list or an invalid position? */

I don't think an assert is appropriate as that forces the program to exit.
But some form of error indication may be useful. Or you can document the behavior and decide what it done. I personally would say (in the documentation) an insert where index is beyond the end of the array is equivalent to a push_back() and leave it at that. Then inserting into an empty list and beyond the end have the same behavior.

Then length as currently implemented:

size_t len( llist* list ) {

Is actually an O(n) operation. It would be a simple to make this O(1) and store the size.

The method that frees the list does not free up the container object:

void free_llist( llist* list ) // frees all the nodes.

I would also expect it to free the object allocated by init_llist.

My first bug bear is identifiers should not begin with an underscore:

typedef struct _node {
typedef struct _llist {

See: what-are-the-rules-about-using-an-underscore-in-a-c-identifier

Rather than having any output parameters I think it is easier to see behavior by returning results:

node* init_node(int value )
{
 node* new_node = (node*)malloc( sizeof(node) );
 new_node->value = value;
 new_node->next = NULL;
 return new_node;
}

/* not sure what I should do here. should I hide the 'error' or force an assert to fail if the caller tries to insert into an empty list or an invalid position? */

I don't think an assert is appropriate as that forces the program to exit.
But some form of error indication may be useful. Or you can document the behavior and decide what it done. I personally would say (in the documentation) an insert where index is beyond the end of the array is equivalent to a push_back() and leave it at that. Then inserting into an empty list and beyond the end have the same behavior.

Then length as currently implemented:

size_t len( llist* list ) {

Is actually an O(n) operation. It would be a simple to make this O(1) and store the size.

The method that frees the list does not free up the container object:

void free_llist( llist* list ) // frees all the nodes.

I would also expect it to free the object allocated by init_llist.

My first bug bear is identifiers should not begin with an underscore:

typedef struct _node {
typedef struct _llist {

See: what-are-the-rules-about-using-an-underscore-in-a-c-identifier

Rather than having any output parameters I think it is easier to see behavior by returning results:

node* init_node(int value )
{
 node* new_node = (node*)malloc( sizeof(node) );
 new_node->value = value;
 new_node->next = NULL;
 return new_node;
}

/* not sure what I should do here. should I hide the 'error' or force an assert to fail if the caller tries to insert into an empty list or an invalid position? */

I don't think an assert is appropriate as that forces the program to exit.
But some form of error indication may be useful. Or you can document the behavior and decide what it done. I personally would say (in the documentation) an insert where index is beyond the end of the array is equivalent to a push_back() and leave it at that. Then inserting into an empty list and beyond the end have the same behavior.

Then length as currently implemented:

size_t len( llist* list ) {

Is actually an O(n) operation. It would be a simple to make this O(1) and store the size.

The method that frees the list does not free up the container object:

void free_llist( llist* list ) // frees all the nodes.

I would also expect it to free the object allocated by init_llist.

Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341

My first bug bear is identifiers should not begin with an underscore:

typedef struct _node {
typedef struct _llist {

See: what-are-the-rules-about-using-an-underscore-in-a-c-identifier

Rather than having any output parameters I think it is easier to see behavior by returning results:

node* init_node(int value )
{
 node* new_node = (node*)malloc( sizeof(node) );
 new_node->value = value;
 new_node->next = NULL;
 return new_node;
}

/* not sure what I should do here. should I hide the 'error' or force an assert to fail if the caller tries to insert into an empty list or an invalid position? */

I don't think an assert is appropriate as that forces the program to exit.
But some form of error indication may be useful. Or you can document the behavior and decide what it done. I personally would say (in the documentation) an insert where index is beyond the end of the array is equivalent to a push_back() and leave it at that. Then inserting into an empty list and beyond the end have the same behavior.

Then length as currently implemented:

size_t len( llist* list ) {

Is actually an O(n) operation. It would be a simple to make this O(1) and store the size.

The method that frees the list does not free up the container object:

void free_llist( llist* list ) // frees all the nodes.

I would also expect it to free the object allocated by init_llist.

lang-c

AltStyle によって変換されたページ (->オリジナル) /