Problem with linked list in C

Hi,
When I call the function Print it loops in endless cycle. I can't solve this problem. Hope somebody can help. Here is the code:

Code:

---

#include <stdio.h>

typedef struct curr
{
        char name[50];
        char code[3];
        float value;
        int number;
        struct curr *next;
}currency;
currency *start_ptr = NULL;

int cnt = 0;

void Print(currency *list)
{
        currency *temp = list;
        if (temp!=NULL)
                while(temp!=NULL)
                {
                                printf("%d.\t%s\t%s\t%f\n", temp->number, temp->name, temp->code, temp->number);
                                temp->next;
                }
        else
                printf("Spisakat e prazen.\n");
}
void InsertItem(currency **list)
{
        currency temp;
        fflush(stdin);
        printf("Vavedi valuta: ");
        gets(temp.name);
        printf("Vavedi kod na valutata: ");
        gets(temp.code);
        printf("Vavedi stoinost za 1USD: ");
    scanf("%f", &temp.value);
                temp.number=++cnt;
        temp.next=NULL;
        *list=&temp;
}

void main()
{
        currency *list = NULL;
        InsertItem(&list);
        Print(list);
}

---

Couple of things, think you are trying merge the actual list and the actual node together in one struct and I don't know if that will work quite right...especially if you start manipulating the list....

I would suggest something to the effect of your current struct being the 'node' and then having a controlling struct that is your list, with essentially two pointers, one to the start of the list, and one as kind of a current location in the list...

The problem with the way you are doing it, as you progress through the list, you will eventually lose the first node in your list....Anyway, it certainly had me having to think about it for a minute...

To answer your specific question...this is why:

Code:

---

        while(temp!=NULL)
                {
                                printf("%d.\t%s\t%s\t%f\n", temp->number, temp->name, temp->code, temp->number);
                                temp->next;
                }

---

You are never changing the pointer of temp....you are referencing temp->next but never setting temp to it...change to:

Code:

---

    temp = temp->next;

---

And I believe your infinite loop will go away; however, as I mentioned above, I think you have some other larger logical issues going on there...not to mention problems with using gets and static buffer sizes ;)

nebulus2000 is right, you should have two separate ones.

First, I am sorry for my later reply, but I was out of town for a while :(
nebulus200, I didn't get you ... I have two structures - temp and list. list i poiting to the start of the list and temp - to the end. Do you mean something like this:

Code:

---

struct *ctrl, *node;
node=list;
ctrl=node;
ctrl->next=temp

---

I googled for this problem and changed the functions in this way:

Code:

---

void Print(currency *list)
{
        if (list!=NULL)
                while(list)
                {
                        printf("%d\t%s\t%s\t%.4f\n", list->number, list->name, list->code, list->number);
                        list=list->next;
                }
        else
                printf("Spisakat e prazen.\n");
}

---

Code:

---

void InsertItem(currency *list)
{
        currency temp, tmp;
        fflush(stdin);
        printf("Vavedi valuta: ");
        scanf("%s", &temp.name);
        printf("Vavedi kod na valutata: ");
        scanf("%s", &temp.code);
        printf("Vavedi stoinost za 1USD: ");
        scanf("%f", &temp.value);
        temp.number=++cnt;
        temp.next=NULL;
        list=&temp;
}

---

But now it gives me exception on this (in print() function):

Code:

---

printf("%d\t%s\t%s\t%.4f\n", list->number, list->name, list->code, list->number);

---

when you use currency *list, you are actually using a pointer to a pointer or in other words a double pointer so that you can actually modify the pointer's location and move it around the list instead of doing a return everytime to the single pointer.

Now coming to the display function, I see no need to use a double pointer as you are not modifying the list pointer.

I will be pasting a code here

Code:

---

void display(currency list)
{
currency temp=list;
        while(temp!=NULL)
        {
          cout<<temp->info<<"\t"; //put your structure members here
          temp=temp->next;
          }
}

---

the problem with your code was that you were using a double pointer and instead of storing the pointer to list in a temp variable, you were doing

Code:

---

if (list!=NULL)
                while(list)
                {
                        printf("%d\t%s\t%s\t%.4f\n", list->number, list->name, list->code, list->number);
                        list=list->next;

---

Since you were using a pointer to the pointer list, the above function should use something like (*list)->number and *list=(*list)->next. But even this is not recommended, as it takes list for a nice trip through the linked list and it would end up at the very last element of the list. So after displaying if you run another function like insertion, it may end up inserting at the rear instead of at the front.

Hope this makes things clear :)

But now it gives me error "binary '=' : no operator defined which takes a right-hand operand of type 'struct curr *' (or there is no acceptable conversion)" on

Code:

---

temp=temp.next;

---

I tried in this way:

Code:

---

temp=*temp. next

---

bu it gives me exception on this row at runtime.

oops, I have made a mistake here. Extremely sorry for that. I didn't notice that the typedef was only for the structure, not for a pointer to the structure.

So everything I wrote in my previous post wont help in this context. Just doing the following SHOULD solve it.

Code:

---

void display(currency *list)
{
currency *temp=list;
            while(temp!=NULL)
            {
              cout<<temp->number<<temp->name<<"\n";
              temp=temp->next;
            }
}

---

Again extremely sorry for the confusion I created.

hey I decided to compile your code and I think you had a logical mistake in the insert function. I wrote it in c++, but I think you won't have much problem in porting it to c.

Code:

---

void InsertItem(currency **list)
{
        currency *temp;
        cout<<"Enter name: ";
        gets(temp->name);
        cout<<"Enter code: ";
        gets(temp->code);
        cout<<"Enter value: ";
        cin>>temp->value;
        temp->number=++cnt;
        temp->next=*list;
        *list=temp;
}

---

when I use this with the display function I posted above, it works perfectly fine.

Note that this function is for insertion at the front. For insertion at the rear, you just have to do the following:

Code:

---

curreny *cur;
cur=*list;
  while(cur->next!=NULL)
      cur=cur->next; //this finds out the last node
cur->link=temp;
temp->link=NULL;

---

And one more thing, I think allocating a fixed memory space for the nodes using malloc or new would result in better memory management.

pi><boy, I tried your way, but when I debug it gives me error "Please enter path for GETS.H"

the code I posted was a c++ code, did you first change it into a standardized c code? It seems it can't find the header file for gets().

#include<stdio.h> should be enough. Dunno about what's wrong now.

Yes, I changed it. Here it is:

Code:

---

void InsertItem(currency **list)
{
        currency *temp;
        fflush(stdin);
        printf("Vavedi valuta: ");
        gets(temp->name);
        printf("Vavedi kod na valutata: ");
        gets(temp->code);
        printf("Vavedi stoinost za 1USD: ");
    scanf("%f", &temp->value);
        temp->number=++cnt;
        temp->next=*list;
        *list=temp;
}

---

I included <strdio.h>

If I do it in this way:

Code:

---

void InsertItem(currency *list)
{
        currency temp;
        fflush(stdin);
        printf("Vavedi valuta: ");
        gets(temp.name);
        printf("Vavedi kod na valutata: ");
        gets(temp.code);
        printf("Vavedi stoinost za 1USD: ");
    scanf("%f", &temp.value);
        temp.number=++cnt;
        temp.next=list;
        list=&temp;
}

---

it works. But still have problem with the print function

Hey how does the insert function work?
You have changed the argument to a single pointer. So it should be

Code:

---

currency* InsertItem(currency *list)
{
//.......

return list;
}

---

and in main() it should be called as

Code:

---

list=InsertItem(list);

---

[EDIT]

here is the whole code after I made some changes. It works totally fine for me except for the fact that it can't do scanf("%f",&temp->value). It shows some "floating point formats not linked" on the borland compiler. But I think you won't be seeing that on something like cc or gcc.

Code:

---

#include <stdio.h>


typedef struct curr
{
        char name[50];
        char code[3];
        float value;
        int number;
        struct curr *next;
}currency;
currency *start_ptr = NULL;

int cnt = 0;

void Print(currency *list)
{


currency *temp=list;
            while(temp!=NULL)
            {
              printf("%d",temp->number);
              temp=temp->next;
            }

}

void InsertItem(currency **list)
{
        currency *temp;
        temp=(currency*)malloc(sizeof(curr));
        fflush(stdin);
        printf("Vavedi valuta: ");
        gets(temp->name);
        printf("Vavedi kod na valutata: ");
        gets(temp->code);

        temp->number=++cnt;
        temp->next=*list;
        *list=temp;

}

void main()
{
        currency *list = NULL;
        InsertItem(&list);
        Print(list);
}

---

Ok, but now I get back to the previous problem - the cycle in the print function loop forever. And another thing - if I call Print(), without called InserItem before, it don't show me "Spisakat e prazen", but gives me an exception on:

Code:

---

while(temp->next!=NULL)

---

Hey see the above edited post ^^

pi><boy, you r the Man! Thanks a lot ... it is done!

Best regards!

you were segfaulting ;)

argh ... another stupid exception. I want to add new items to the edn of the list and resource the function:

Code:

---

void InsertItem(currency **list)
{
        currency *temp, *node;
        temp=(currency*)malloc(sizeof(curr));
        node=*list;
        while(node)
                node=node->next;
        fflush(stdin);
        printf("Vavedi valuta: ");
        gets(temp->name);
        printf("Vavedi kod na valutata: ");
        gets(temp->code);
        printf("Vavedi stoinost za 1USD: ");
    scanf("%f", &temp->value);
        temp->number=++cnt;
        temp->next=NULL;
        node->next=temp;
        *list=node;
}

---

And now it gives me exception on this row:

Code:

---

        node->next=temp;

---

Hmmm ... obviously I have problems with these pointers :(

yeah you are having problems with pointers. I suggest you to do a bit more of reading up.

I think it would be

Code:

---

while(node->next!=NULL)
            node=node->next;

---

Your code is while(node). That wouldn't take the pointer node to the last node in the list. Rather it would take you to the link of the last node i.e. NULL and only then will the while loop stop working.

Nope, I tried it already and gives me exception on:

Code:

---

while(node->next!=NULL)

---

It don't allows me to change node->next. I can change it only before node=*list;

Here's the working code:

Code:

---

void InsertItem(currency **list)
{
        currency *temp, *node;
        temp=(currency*)malloc(sizeof(curr));

        fflush(stdin);
        printf("Vavedi valuta: ");
        gets(temp->name);
        printf("Vavedi kod na valutata: ");
        gets(temp->code);
        printf("Vavedi stoinost za 1USD: ");
        scanf("%f", &temp->value);
        temp->number=++cnt;
        temp->next=NULL;
        node=*list;
        if(node!=NULL) {
                while(node->next!=NULL)
                        node=node->next;
                node->next=temp;
        }
        else
        *list=temp;
}

---

You gotta take into account the fact that when no node is created in the beginning, there are no nodes to traverse.

Ah ... yes, of course ... such a stupid mistake ...