Why won't my program work?

This is a discussion on Why won't my program work? within the C Programming forums, part of the General Programming Boards category; I tried to make a linked list and it won't work, what's wrong with it? Code: #include <stdio.h> void main(void) ...

  1. #1
    Unregistered
    Guest

    Why won't my program work?

    I tried to make a linked list and it won't work, what's wrong with it?
    Code:
    #include <stdio.h>
    
    void main(void)
    {
      struct node
      {
        char *name;
        int count;
        struct node *next;
      };
    
      struct node root = {"Start", 1, 0};
    
      while(root.count < 20)
      {
        root->next = malloc(sizeof(root));
        root.name = "Benny Hill";
        root.count += 1;
        root->next = NULL;
      }
    
      while(root != NULL)
      {
        printf("%s", root.name);
        root = root->next;
      }
    }

  2. #2
    Unregistered
    Guest
    you didn't declare name as a string. And when you use printf, you used %s.

  3. #3
    ATH0 quzah's Avatar
    Join Date
    Oct 2001
    Posts
    14,826
    >> Why won't my program work?
    >
    > you didn't declare name as a string. And when you use printf,
    > you used %s.

    What? Yes they did. Do you even know what a string is?

    >> char *name;

    That is a "string", which happens to be called... "name". Fancy that?

    The real problem is this:

    >> root = root->next;

    'root' is not a pointer, so this assignment is not valid.

    Quzah.
    Hope is the first step on the road to disappointment.

  4. #4
    Unregistered
    Guest
    It runs now, but nothing prints out.
    Code:
    #include <stdio.h>
    #include <stdlib.h>
    
    void main(void)
    {
      struct node
      {
        char *name;
        int count;
        struct node *next;
      };
    
      struct node *root = malloc(sizeof(root));
      root->name = "Start";
      root->count = 1;
    
      while(root->count < 5)
      {
        root->next = malloc(sizeof(root));
        root->name = "Benny Hill";
        root->count += 1;
        root->next = NULL;
      }
    
      while(root->next != NULL)
      {
        printf("%s %d", root->name, root->count);
        root = root->next;
      }
    }

  5. #5
    B26354 Deckard's Avatar
    Join Date
    Jan 2002
    Posts
    633

    Re: Why won't my program work?

    Code:
    while(root.count < 20)
    {
      root->next = malloc(sizeof(root));
      root.name = "Benny Hill";
      root.count += 1;
      root->next = NULL;
    }
    There are also some problems here. I'd like to focus on the logic errors, so I will fix the grammar:

    Code:
    while( root.count < 20 )
    {
      root.next = (struct node *) malloc( sizeof(root) );
      strncpy( root.name, "Benny Hill", sizeof(root.name) );
      root.count += 1;  /* could also be root.count++ if you prefer */
      root.next = NULL;
    }
    I suspect you intended to a) allocate memory for a new node structure, b) point 'next' to the newly allocated memory, c) name the new link entry "Benny Hill", d) set 'count' in the new structure to be one greater than the current count, and e) set the new structure's 'next' pointer to NULL.

    If we look carefully at the block of code, we see that you do indeed allocate memory for a new list element, and give it a 'name'. Then, instead of incrementing the count in the new element, you increment the count in the original element. Remember that 'root' is the original element and, for reasons quzah pointed out, 'root' will always be the original element (it is not a pointer you can step through the list with). Then you set 'next' (which was just pointed to the new element) to NULL. You allocate memory on the heap and then set the only pointer to that memory to NULL, which creates a memory leak (the allocated memory is simply 'forgotten', not freed). You then loop through this all over again starting with the same 'root' structure (creating and losing a whole new area of memory).

    All hope is not lost. Make root a pointer you can step through your list with and be sure step through your list while allocating new nodes so you don't create memory leaks.

    Best of luck.
    Jason Deckard

  6. #6
    Banned Troll_King's Avatar
    Join Date
    Oct 2001
    Posts
    1,784
    Here is an example of a list with a couple functons defined such as Addnode and Display.

    Code:
    #include <stdio.h> 
    #include <string.h>
    #include <stdlib.h>
    
    typedef struct node_s
    {
        char * name;
        node_s *next;
    }Node_t;
    
    void Addnode(Node_t **);
    void Display(const Node_t *);
    
    int main()
    {
    	Node_t *list = NULL;
    
    	const size = 20;
    
    
    	Addnode(&list);
    	Addnode(&list);
    	Display(list);
    
    	return 0;
    }
    
    
    void Addnode( Node_t **hp)
    {
    	Node_t *newp;
    	const size = 20;
    	char newname[size];
    
    	printf("Enter new name: ");
    	fgets(newname,20,stdin);
    	newname[strlen(newname) - 1] = '\0';
    
    	if(*hp == NULL)
    	{
    		*hp = (Node_t *) malloc (sizeof(Node_t));
    		(*hp)->next = NULL; 
    		(*hp)->name = (char *) malloc (sizeof(char) * size);
    
    		strcpy((*hp)->name,newname);
    		printf("hp name %s\n", (*hp)->name);
    	}
    	else
    	{
    		newp = (Node_t *) malloc (sizeof(Node_t));
    		newp->next = NULL;
    		newp->name = (char *) malloc (sizeof(char) * size);
    	
    		strcpy(newp->name, newname);
    		printf("newp name %s\n", newp->name);
    
    		newp->next = *hp;
    		*hp = newp;
    	}
    
    }
    
    void Display(const Node_t *hp)
    {
    	while(hp)
    	{
    		printf("\nNode name member is %s", hp->name);
    		hp = hp->next;
    	}
    }

  7. #7
    Unregistered
    Guest
    I tried what you suggested and it still doesn't work, but now it just goes into an infinite loop and doesn't do anything, I have to end the program forcefully.
    Code:
    #include <stdio.h>
    #include <stdlib.h>
    #include <string.h>
    
    void main(void)
    {
      struct node
      {
        char *name;
        int count;
        struct node *next;
      };
    
      struct node *root = malloc(sizeof(struct node));
      struct node *top = root;
      strcpy(root->name, "Start");
      root->count = 1;
      root->next = NULL;
    
      while( root->count < 20 )
      {
        root->next = (struct node *) malloc( sizeof(root) );
    	root = root->next;
        strncpy( root->name, "Benny Hill", sizeof(root->name) );
        root->count += 1;
        root->next = NULL;
      }
    
      root = top;
      while(root->next != NULL)
      {
        printf("%s %d", root->name, root->count);
        root = root->next;
      }
      free(root);
      free(top);
    }

  8. #8
    Banned Troll_King's Avatar
    Join Date
    Oct 2001
    Posts
    1,784
    Malloc to sizeof(struct node) rather than root. Or else notice the code I posted. Try to follow the logic of adding a node to the list.

  9. #9
    Unregistered
    Guest
    Is anyone even running this code to see what it's doing? So far none of your suggestions have worked, now the program doesn't even end, I have to shut it down manually. Granted, the first suggestion got rid of the errors I had but from that point on it just seems like you're changing things to suit your style and not effecting my program at all.
    I know that you're here answering my question because you want to and I appreciate that very much, but please answer the question on the first try if possible

  10. #10
    Unregistered
    Guest
    Troll_King: I did notice the code you posted, I read through it thoroughly and understood almost none of it.

  11. #11
    ATH0 quzah's Avatar
    Join Date
    Oct 2001
    Posts
    14,826
    > Is anyone even running this code to see what it's doing?

    I'm not. I don't need to. It's obvious what your problems are, which is why we're pointing them out. Perhaps you should use a little patients, and take a few moments to reflect on the comments made.

    > So far none of your suggestions have worked, now the
    > program doesn't even end, I have to shut it down manually.

    Well then, you're hardly worse off, sicne your code didn't work in the first place.

    I've already told you what's wrong.

    1) make a root pointer as a place holder, allocate a node
    2) make another pointer as to use to manipulate data
    3) use a loop to make your nodes and input data:
    Code:
    for(ptr = rootptr, x = 0; x < some number; x++ )
    {
        fillptrwithdata;
        ptr = ptr->next;
    }
    4) Use the exact same thing to display your data:
    Code:
    for( ptr = root; ptr; ptr = ptr->next )
    {
        printf( "%s", ptr->name );
    }
    It's not hard.

    Quzah.
    Hope is the first step on the road to disappointment.

  12. #12
    Banned Troll_King's Avatar
    Join Date
    Oct 2001
    Posts
    1,784
    Okay, maybe this will help. When you create a linked list, think of each link in the list as a node. Basically a node is a structure, it's a record of information.

    If you have a file full of records than each record fills one node. In order to organize all the nodes together in computer memory they have to be linked with pointers. Each pointer is like a chain link to the next node.

    Looking at your original code. The problem with it is that you need more nodes. You just have one node and you tried to add more nodes, but it was done wrong. You started correctly. You have to start with one node, but then for the second node and the rest of the nodes, they much be distinct nodes, which get connected to the list. That is the biggest single most mistake of your original program.

    The way you connect nodes is with pointer. In other words you need more tools if you want to connect nodes. Since this is an abstract concept I suggest you read the chapter from the textbook again. Try a different textbook even, and come back to the program. Otherwise you will just get frustrated. Lists are not easy until you can conceptualize the the nodes, the connectors, and the list.

    BTW I can't spell!
    Last edited by Troll_King; 01-10-2002 at 08:32 PM.

  13. #13
    Registered User
    Join Date
    Dec 2001
    Posts
    421
    Is anyone even running this code to see what it's doing? So far none of your suggestions have worked

    geez. you sure do have a way with people when you are trying to get them to help you.

    Don't forget your manners here mate, these guys are helping you, and the points they have made are valid. quzah is right, we dont' need to run your code to see what's wrong...

    keep in mind that YOU are the one with the problem, not us.

    Thanks
    U.
    Quidquid latine dictum sit, altum sonatur.
    Whatever is said in Latin sounds profound.

  14. #14
    Unregistered
    Guest
    I got the list to work, thank you all for your help despite my attitude. I do have one more simple question though, and then I'll take my leave of you forever, that should make you happy.

    I was told that to fully free the list I need to delete each node, so I wrote a loop to do that. However, when I run it I get an assertion error in visual c++ 6. After debuggin this is the piece of code that is causing the problem but I have no idea where or how. Here's the code
    Code:
    /*destroy the list*/
    ptr = rootPtr;
    for(nextPtr = ptr->next; nextPtr; nextPtr = ptr->next)
    {
      free(ptr);
      ptr = nextPtr;
    }

  15. #15
    B26354 Deckard's Avatar
    Join Date
    Jan 2002
    Posts
    633
    Originally posted by Unregistered
    I do have one more simple question though, and then I'll take my leave of you forever, that should make you happy.
    There's no reason for you to become a C Board hermit after this ;)

    Code:
    /*destroy the list*/
    ptr = rootPtr;
    for(nextPtr = ptr->next; nextPtr; nextPtr = ptr->next)
    {
      free(ptr);
      ptr = nextPtr;
    }
    The for loop starts off with 'ptr' pointing to the first node of the list (I assume that is what 'rootPtr' is), and with 'nextPtr' pointing to 'next' element of 'ptr' (this is done in the initialization section of the for statement).

    Then we check to see if nextPtr has a non-NULL value. This will be true if there are two or more nodes in the list, but if you have a one-node list, that one node will never be freed (since there is no second node, 'nextPtr' is NULL and the for statement terminates before free() is called).
    Jason Deckard

Page 1 of 2 12 LastLast
Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Client-server system with input from separate program
    By robot-ic in forum Networking/Device Communication
    Replies: 3
    Last Post: 01-16-2009, 02:30 PM
  2. BOOKKEEPING PROGRAM, need help!
    By yabud in forum C Programming
    Replies: 3
    Last Post: 11-16-2006, 10:17 PM
  3. threaded program doesn't work?
    By OcTO_VIII in forum Linux Programming
    Replies: 1
    Last Post: 12-11-2003, 11:37 AM
  4. The Bludstayne Open Works License
    By frenchfry164 in forum A Brief History of Cprogramming.com
    Replies: 8
    Last Post: 11-26-2003, 10:05 AM
  5. how does this program work
    By ssjnamek in forum C++ Programming
    Replies: 2
    Last Post: 01-02-2002, 12:48 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21