Thread: comment on my comments

Only thing I would change is:

node1->next=node2; /* node1 structure pointer next is assigned the address of */
/* of the first element of the second structure */

To this:

Code:

node1->next=node2; /* first nodes' next pointer is assigned the address of */ 
/* of the second node */

Yes, but it would seem to me that saying it the way you suggested, that the pointer isn't a pointer to a node but rather an element of a node. For me, I would rather have it the other way, saying that one node's pointer member points to the next node makes the point that this is a linked list and not something else just a little bit clearer in my mind. Anyways, I guess its all just a matter of semantics, choose whatever one you want.

Moreover, this is not really a linked list as such. Truly, a linked list(LL) is a method for easy traversal of some data and also a 'dynamic' mechanism which allows the addition and removal of containers as needed. However, unlike arrays, the manipulations of these structures requires that the programmer write new functions for each new LL type. And it is the functions that are first in order for development. You will need:
1) A 'new' function(a thin encapsulation of malloc)
2) A 'delete' function(you must dealloc not just for the structure, but for any char *'s you may have. Plus you must reconnect the list where the node is deleted)
3) An 'addnode' function(to connect it to the existing list)
4) One or more 'sort' functions(sort by some key)
5) One or more 'search/find' functions(find by key)

Many more and useful functions are possible, but those are the minimum.

Incidentally, there is one way to use 'generic' functions, but there's a catch. If you write all your functions to handle a structure type 'Node', say, you can use them with any ONE structure if you make this declaration:

Pretend you have a structue called 'People':

typedef struct People
{
People *prev;
char *name;
int age;
People *next;
};

Then after that declaration, do this:

typedef People Node;

NOW, so long as you do not try to typedef another structure as type 'Node' you can use the generic functions you wrote. Again you are limited to ONE type "Node" in each program.
I hope what I've written is helpful. Good luck and enjoy using Linked Lists, the most powerful container of them all!

This is a prime example of over commenting to the point of confusion. Most of your comments are redundant because the code says the same thing very obviously. There are several things you could do to get rid of the comments in this program, but here are my suggestions. Note that I'm adding a lot more comments than I normally would to show you some key points.

Code:

#include<stdio.h> 
#include<stdlib.h> 
#include<string.h> 

/* It's fairly obvious what this struct is and does
** so why bother commenting it?
*/
struct node{ 
  char word[20]; 
  struct node *next;
};

int main() 
{ 
  struct node *dummyHead, 
              *list;

  /* You really only need to malloc one pointer here
  ** since you just assign the other one later on
  */
  dummyHead = malloc( sizeof *dummyHead );

  /* Don't ever just end the program if it breaks, let 
  ** the user know that something went wrong first
  */
  if( dummyHead == NULL ) {
    printf( "ERROR: Insufficient memory!" );
    return EXIT_FAILURE;
  }

  strcpy( dummyHead->word, "anthony" );
  dummyHead->next = NULL;

  /* Build the list with only two nodes to test it
  ** so a loop is not required
  */
  list = dummyHead;
  list->next = malloc( sizeof *list );
  list = list->next;
  strcpy( list->word, "Cassandra" );
  list->next = NULL;

  /* Print the list
  */
  list = dummyHead;
  while( list != NULL ) {
    printf( "%s\n", list->word );
    list = list->next;
  }

  /* There is a problem here if you can see it.
  ** What happens to the rest of the list if you
  ** have more than two nodes? Only the dummy
  ** node and the last node in the actual list
  ** get freed with this code. Oops? You need to
  ** iterate through the list and free every node
  */
  free(dummyHead); 
  free(list); 

  return EXIT_SUCCESS; 
}

Sebastiani:
>And it is the functions that are first in order for development.
Not if you're just learning linked lists, modularizing it at this point would just make things even more confusing since a simple linked list really only need take up about 17 lines and still do something useful.

-Prelude

Not if you're just learning linked lists, modularizing it at this point would just make things even more confusing since a simple linked list really only need take up about 17 lines and still do something useful.

I disagree. The functions allow the programmer to step back and see the bigger picture. For one, in a linked list, the 'head' must have a higher scope than the node 'spawner'. Without functions, this can become a mess of code and frankly MUCH more confusing than your '17 lines of code'...
Look:

Node *head = NULL;
do
{
clrscr();
printf("Please enter another employee or type 'quit' ");
fgets(input, 100, stdin);
if(strstr(input, "exit"))
break;

Node *born = NewNode(); // declared locally
AddNode(born, head);
strncpy(born->name, input, 100)
}while(1);

clrscr();

PrintNames(head);

DeleteList(head);

return 0;

As you can see, the functions allow a cleaner abstraction.

>As you can see, the functions allow a cleaner abstraction.
There's really no need for abstraction when your program is so small. Granted, abstraction and modularization are very important in professional programs but toy programs for learning shouldn't use concepts that hide the overall structure of the list.

A linked list isn't an easy concept to figure out, but stepping through a 'simple' program with simple actions will work wonders for understanding how the concept of a linked list works.

Code:

#include <stdio.h>
#include <stdlib.h>

struct node
{
  int item;
  struct node *next;
};

int main( int argc, char **argv )
{
  int i, 
      N = atoi( argv[1] ), 
      M = atoi( argv[2] );
  struct node *t = malloc( sizeof *t ), 
              *x = t;
  t->item = 1; 
  t->next = t;

  for( i = 2, i <= N; i++ ) {
    x = ( x->next = malloc( sizeof *x ) );
    x->item = i; 
    x->next = t;
  }

  while( x != n->next ) {
    for( i = 1; i < M; i++ )
      x = x->next;
    x->next = x->next->next;
    N--;
  }

  printf( "%d\n", x->item );

  return EXIT_SUCCESS;
}

-Prelude

True, I guess you're right as far as grasping the list. Still, when I was learning them, I was quick to write the functions FIRST to avoid mistakes and to automate more quickly.
Good point, though.