Skip to main content
Added note to clarify my preference for type *var
Source Link
William Morris
  • 9.4k
  • 19
  • 43

Here's a few comments to add to those of @uli (+1)

Would one ever use a linked list in c++ when there are nice STL containers available for such things? Seems unlikely, but I'm not strong on c++. But in a c++ exercise on the subject of linked lists, I'd expect to see some use of c++ facilities.

For example you could usefully use exceptions to avoid the ambiguity in your head_return and tail_return functions. These two currently return 0 on failure but this is a valid value for the payload (you don't check for it when inserting, so I assume it is a valid payload). You might also use an exception when doing an ordered insert (as opposed to an assert suggested by @uli).

I would change some of your variable names. For example you have three names for a payload, payload, pload and temp. Why not use just one consistently if possible? And you also use temp for a temporary node, but node would be more descriptive.

Here are some detailed comments (some are pedantic and others perhaps only opinion):

  • importing everything from std (using namespace std) is best avoided.

  • I prefer to see type *var rather than type* var - consider the type for var2 in the declaration type* var1, var2. EDIT: as noted by @LokiAstari in the comments, this is a preference carried over from C and is not applicable to C++, where type* var is the convention.

  • you don't have many comments, but those you have are noise (no useful information) and should be deleted. Write useful comments - don't state the obvious.

  • empty loops should be made explicit with braces:

      for (temp = *list; temp->next; temp = temp->next) {
          /* loop */
      }
    
  • ending a function with

      else
      {
          return 0;
      }
    

    is better written as just return 0

  • where possible, define loop variables in the loop:

      for (node_ll *temp = *list; temp->next; temp = temp->next) {...}
    
  • in ordered_insert you'd be better to break the loop by adding a !inserted condition once you've inserted the value rather than continuing to loop:

      for (temp = *list; temp->next && !inserted; temp = temp->next)
    
  • I'm sure that a new_node() function would be useful:

      static node_ll* new_node(node_ll *next, int payload)
      {
          node_ll *node = new node_ll;
          node->payload = payload;
          node->next = *next;
          return node;
      }
    
  • is it really expected that find_remove removes all occurrences of a payload instead of just one?

Here's a few comments to add to those of @uli (+1)

Would one ever use a linked list in c++ when there are nice STL containers available for such things? Seems unlikely, but I'm not strong on c++. But in a c++ exercise on the subject of linked lists, I'd expect to see some use of c++ facilities.

For example you could usefully use exceptions to avoid the ambiguity in your head_return and tail_return functions. These two currently return 0 on failure but this is a valid value for the payload (you don't check for it when inserting, so I assume it is a valid payload). You might also use an exception when doing an ordered insert (as opposed to an assert suggested by @uli).

I would change some of your variable names. For example you have three names for a payload, payload, pload and temp. Why not use just one consistently if possible? And you also use temp for a temporary node, but node would be more descriptive.

Here are some detailed comments (some are pedantic and others perhaps only opinion):

  • importing everything from std (using namespace std) is best avoided.

  • I prefer to see type *var rather than type* var - consider the type for var2 in the declaration type* var1, var2

  • you don't have many comments, but those you have are noise (no useful information) and should be deleted. Write useful comments - don't state the obvious.

  • empty loops should be made explicit with braces:

      for (temp = *list; temp->next; temp = temp->next) {
          /* loop */
      }
    
  • ending a function with

      else
      {
          return 0;
      }
    

    is better written as just return 0

  • where possible, define loop variables in the loop:

      for (node_ll *temp = *list; temp->next; temp = temp->next) {...}
    
  • in ordered_insert you'd be better to break the loop by adding a !inserted condition once you've inserted the value rather than continuing to loop:

      for (temp = *list; temp->next && !inserted; temp = temp->next)
    
  • I'm sure that a new_node() function would be useful:

      static node_ll* new_node(node_ll *next, int payload)
      {
          node_ll *node = new node_ll;
          node->payload = payload;
          node->next = *next;
          return node;
      }
    
  • is it really expected that find_remove removes all occurrences of a payload instead of just one?

Here's a few comments to add to those of @uli (+1)

Would one ever use a linked list in c++ when there are nice STL containers available for such things? Seems unlikely, but I'm not strong on c++. But in a c++ exercise on the subject of linked lists, I'd expect to see some use of c++ facilities.

For example you could usefully use exceptions to avoid the ambiguity in your head_return and tail_return functions. These two currently return 0 on failure but this is a valid value for the payload (you don't check for it when inserting, so I assume it is a valid payload). You might also use an exception when doing an ordered insert (as opposed to an assert suggested by @uli).

I would change some of your variable names. For example you have three names for a payload, payload, pload and temp. Why not use just one consistently if possible? And you also use temp for a temporary node, but node would be more descriptive.

Here are some detailed comments (some are pedantic and others perhaps only opinion):

  • importing everything from std (using namespace std) is best avoided.

  • I prefer to see type *var rather than type* var - consider the type for var2 in the declaration type* var1, var2. EDIT: as noted by @LokiAstari in the comments, this is a preference carried over from C and is not applicable to C++, where type* var is the convention.

  • you don't have many comments, but those you have are noise (no useful information) and should be deleted. Write useful comments - don't state the obvious.

  • empty loops should be made explicit with braces:

      for (temp = *list; temp->next; temp = temp->next) {
          /* loop */
      }
    
  • ending a function with

      else
      {
          return 0;
      }
    

    is better written as just return 0

  • where possible, define loop variables in the loop:

      for (node_ll *temp = *list; temp->next; temp = temp->next) {...}
    
  • in ordered_insert you'd be better to break the loop by adding a !inserted condition once you've inserted the value rather than continuing to loop:

      for (temp = *list; temp->next && !inserted; temp = temp->next)
    
  • I'm sure that a new_node() function would be useful:

      static node_ll* new_node(node_ll *next, int payload)
      {
          node_ll *node = new node_ll;
          node->payload = payload;
          node->next = *next;
          return node;
      }
    
  • is it really expected that find_remove removes all occurrences of a payload instead of just one?

Source Link
William Morris
  • 9.4k
  • 19
  • 43

Here's a few comments to add to those of @uli (+1)

Would one ever use a linked list in c++ when there are nice STL containers available for such things? Seems unlikely, but I'm not strong on c++. But in a c++ exercise on the subject of linked lists, I'd expect to see some use of c++ facilities.

For example you could usefully use exceptions to avoid the ambiguity in your head_return and tail_return functions. These two currently return 0 on failure but this is a valid value for the payload (you don't check for it when inserting, so I assume it is a valid payload). You might also use an exception when doing an ordered insert (as opposed to an assert suggested by @uli).

I would change some of your variable names. For example you have three names for a payload, payload, pload and temp. Why not use just one consistently if possible? And you also use temp for a temporary node, but node would be more descriptive.

Here are some detailed comments (some are pedantic and others perhaps only opinion):

  • importing everything from std (using namespace std) is best avoided.

  • I prefer to see type *var rather than type* var - consider the type for var2 in the declaration type* var1, var2

  • you don't have many comments, but those you have are noise (no useful information) and should be deleted. Write useful comments - don't state the obvious.

  • empty loops should be made explicit with braces:

      for (temp = *list; temp->next; temp = temp->next) {
          /* loop */
      }
    
  • ending a function with

      else
      {
          return 0;
      }
    

    is better written as just return 0

  • where possible, define loop variables in the loop:

      for (node_ll *temp = *list; temp->next; temp = temp->next) {...}
    
  • in ordered_insert you'd be better to break the loop by adding a !inserted condition once you've inserted the value rather than continuing to loop:

      for (temp = *list; temp->next && !inserted; temp = temp->next)
    
  • I'm sure that a new_node() function would be useful:

      static node_ll* new_node(node_ll *next, int payload)
      {
          node_ll *node = new node_ll;
          node->payload = payload;
          node->next = *next;
          return node;
      }
    
  • is it really expected that find_remove removes all occurrences of a payload instead of just one?