Thread: Help guys with circluarly linked list

I want to make a circularly linked list taking data from a text file.
It keeps saying that my list is Empty when i try to print it. Is it really empty or is something wrong with the printlist function?


So far I have written this:

Code:

Lics()
{
    struct p_item *mylist = NULL;
    char letter;
    char word[15];
    int i, j, length;
    FILE *phonetics;
    char c[40];
    
    i=0;
    phonetics=fopen("ICAO.phon", "r");
    if (phonetics == NULL){ 
        printf ("Error opening file\n");
        return;
    }
    else {
        while(!feof(phonetics)) { 
            fscanf(phonetics, "%c", &letter);
            
            fscanf(phonetics, "%s", word);
            
            addnode(mylist, letter, word);
        }
    }
    printf("\n\n");
    printlist(mylist);
    
    return;
} 
 
addnode(struct p_item *list, char *letter, char *word[15])
{
    struct p_item *newnode, *first=list;
    
    if (list==NULL){
        newnode=(struct p_item *)malloc(sizeof(struct p_item));
        newnode->p_letter=letter;
        strcpy(newnode->p_word, word);
        newnode->p_next=newnode;
        return newnode;
    }
    
    while (list->p_next != first){
        list = list->p_next;
        }
        
    newnode = (struct p_item *)malloc(sizeof(struct p_item));
    list->p_next = newnode;
    newnode->p_letter=letter;
    strcpy(newnode->p_word, word);
    newnode->p_next = first;
 
    return first;
    
}

printlist(struct p_item *list)
{
    struct p_item *first = list;
    
    if (list == NULL){
        printf("The circularly linked list is empty!\n");
        return;
    }
    do{
        printf("%c %s\n", list->p_letter, list->p_word);
        list = list->p_next;
        } while(list != first);
    
}

In Lics()? you set mylist = NULL, and in your call to addnode() and in addnode() itself, you never change mylist to point to a struct, so statements like

Code:

while (list->p_next != first){        list = list->p_next;
        }

and

Code:

list->p_next = newnode;

make no sense.

You are returning values but not catching them anywhere, and your function addnode() doesn't have a return type. So locals newnode and first are lost on exit of addnode() and each time you test your program you are malloc'ing memory which you never free.

Thx for your reply but I have to admit that although i see your point, I can't exactly make it happen. Please consider that I have been essentially working on C for about one month (and not that often in that month) so my experience is rather inadequate.

Could you please point to the proper line of the code and suggest what I should do? This is just the first of my problems but if I can't solve this, I won't be able to go further on.

I must be so inexperienced or so "bad" at C that I still don't understand what I must do in order to get the list to fill itself with what I want. Please show me the way!!

Btw, I totally get the one about feof (at least I understood that one)

Originally Posted by iMalc

Your first step would be to give your functions return types. Don't just let them default to int.

Calling a Function and Writing Your Own Functions

Originally Posted by Tclausex

In Lics()? you set mylist = NULL, and in your call to addnode() and in addnode() itself, you never change mylist to point to a struct, so statements like

Code:

while (list->p_next != first){        list = list->p_next;
        }

and

Code:

list->p_next = newnode;

make no sense.

What should I do about these things mentioned above?

Originally Posted by lios1984

What should I do about these things mentioned above?

Fix addnode function so the list head (mylist) is updated.

Tim S.

I have isolated the problem to the part )if (list==NULL). I realize that I can't get to fill the first node of the list. But why? Please consider that you are talking to a C illiterate.

What should I return;

Code:

addnode(struct p_item *list, char *letter, char *word)
{
    struct p_item *newnode, *first=list;
    
    if (list==NULL){
        printf("Test1");
        list=(struct p_item *)malloc(sizeof(struct p_item));
        list->p_letter=letter;
        strcpy(list->p_word, word);
        list->p_next=first;
        if (list==NULL){
            printf("Error in memory allocation.");
        }
        return list;
    }
    
    while (list->p_next != first){
        list = list->p_next;
        }   
    printf("Test2");
    newnode = (struct p_item *)malloc(sizeof(struct p_item));
    list->p_next = newnode;
    newnode->p_letter=letter;
    strcpy(newnode->p_word, word);
    newnode->p_next = first;
    if (newnode==NULL){
        printf("Error in allocating memory");
    }
    
    return list;

    
}

1. Change addnode so that its return value is 'struct p_item *'

2. Check for malloc failure BEFORE you attempt to start assigning things.

3. Don't use implicit cast on malloc.

4. Realize that your second half of addnode returns the address of the second to last struct in the list.

5. Your parameter for addnode() should be char letter, not char *letter.

And lastly, turn you compiler warnings on. Lastly should really be, why are you playing with structs, pointers and linked lists, 1 month out of the gate?

Guys, don't even ask!! I am trying to complete a course. So far I have managed to get by regarding C. But now with malloc and linked lists, I am way over my head. What I would really appreciate is a step-by-step through my code so that I can finally realize what I am doing so wrong. Ia give you the whole code in case something's wrong with declaring or something else.

Code:

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

struct p_item
{
    char p_letter;
    char p_word[15];
    struct p_item *p_next;
};

typedef struct p_item p_item;
   

int main(int argc, char *argv[])
{
    char c;
    int i, j;
    
    Lics();
    printf("\n");
    
    system("pause");
    return 1;
}

Lics()
{
    struct p_item *mylist = NULL;
    char letter;
    char word[15];
    int i, j, length;
    FILE *phonetics;
    char c[40];
     
    i=0;
    phonetics=fopen("ICAO.phon", "r");
    if (phonetics == NULL){
        printf ("Error opening file\n");
        return;
    }
    else {
        while(!feof(phonetics)) {
            fflush(stdin);
            fscanf(phonetics, "%c", &letter);
             
            fscanf(phonetics, "%s", word);
             
            struct p_item *addnode(mylist, letter, word);
        }
    }
    printf("\n\n");
    printlist(mylist);
     
    return;
}
  
struct p_item *addnode(struct p_item *list, char *letter, char *word[15])
{
    struct p_item *newnode, *first=list;
     
    if (list==NULL){
        list=(struct p_item *)malloc(sizeof(struct p_item));
        newnode->p_letter=letter;
        strcpy(newnode->p_word, word);
        newnode->p_next=newnode;
        return newnode;
    }
     
    while (list->p_next != first){
        list = list->p_next;
        }
         
    newnode = (struct p_item *)malloc(sizeof(struct p_item));
    list->p_next = newnode;
    newnode->p_letter=letter;
    strcpy(newnode->p_word, word);
    newnode->p_next = first;
  
    return first;
     
}
 
printlist(struct p_item *list)
{
    struct p_item *first = list;
     
    if (list == NULL){
        printf("The circularly linked list is empty!\n");
        return;
    }
    do{
        printf("%c %s\n", list->p_letter, list->p_word);
        list = list->p_next;
        } while(list != first);
     
}

Don't pay attention to the main() part as well as to the !feof part, for now. I mostly want to get over with the list and then the former.

Originally Posted by Tclausex

1. Change addnode so that its return value is 'struct p_item *'

2. Check for malloc failure BEFORE you attempt to start assigning things.

3. Don't use implicit cast on malloc.

4. Realize that your second half of addnode returns the address of the second to last struct in the list.

5. Your parameter for addnode() should be char letter, not char *letter.

And lastly, turn you compiler warnings on. Lastly should really be, why are you playing with structs, pointers and linked lists, 1 month out of the gate?

1. Did it.
2. I am looking it up right now.
3. What do you mean?
4. Theoretically I get it but practically...
5. When I use char *letter it runs but leads to the error message (The circularly linked list is empty!) and when I use char letter, it doesn't run.

!!!!
I got only 2 warnings whenever I compiled. I use wx-Dev-C++. Maybe that's because I don't get all these warnings? Or are they turned off? I can't find any settings.

How do I turn them on?

I'll stay up tonight and try to solve this. So I'll be online..

Originally Posted by lios1984

.
4. Theoretically I get it but practically...

Well, you wrote the code to move 'list' up to the last struct in the linked list, and then malloc a new struct, and point list->p_next to that new struct. So now list points to that second-to-last struct and that memory location is what your are returning to main to be assigned to 'mylist'. (At least I assume you're assigning it to mylist).

I guess I'm asking if that's really what you want. Think about it for a few minutes. You never really have a "first" item in the linked list, it's constantly changing. And every addnode() call treats that position as "first" and then runs around the ring until it is behind "first". As your list grows, you are literally running around in circles for no good reason. You need to decide if this is really the behavior you want, or do you want the first struct added to really be, and remain the "first" node.

Originally Posted by lios1984

.
5. When I use char *letter it runs but leads to the error message (The circularly linked list is empty!) and when I use char letter, it doesn't run.

Trust me, you can use char *letter, but it's not called for here. You use a pointer as parameter in a function when you want to change the variable in main() from within said function. You aren't changing letter, so a pointer is uncalled for. And IF you wanted to use a pointer, the value you pass to it with the function call must be an address, i.e. use the & address operator.
Using the correct formal parameter of 'char letter' is certainly NOT what is preventing your program from working.

Finally, I see you removed the malloc error-checking altogether . Put it back or puppies will die. The error checking should come right after the malloc call, otherwise you attempt to assign values to a struct that doesn't exist. And if malloc returns NULL, frankly, you should end your program with an error statement and an exit(1) call.