Thread: Putting info in a Struct

Hey guys, I'm having a little problem where after reading a file I'm unable to update my struct with some particular info. It is actually a linkedlist.

This is my struct:

Code:

struct node{
    int id;
    char name[50];
    struct node *next;
}*head;

And in my Main function:

Code:

int  main(){
    int i, t_id, num;
    struct node *temp;
    char name[50], t_name[50], input_name[50], line[LINESIZE], *value;
    struct node *head = NULL;
    FILE * ifp = fopen("AssignmentOneInput.txt", "r");




    while ((fgets(line, sizeof(line), ifp)))
        {
            value = strtok(line, ",");
            num = strtok(NULL, "\n");
            
        }

Basically in the while loop when I input something like this for updating numbers:

Code:

temp->id = num;  //These are numbers separated by a comma.

or this:

Code:

strcpy(temp->name,value);  //These are names

The program crashes, any help would be appreciated, thank you!

Originally Posted by laserlight

This is bad practice:

Code:

struct node{
    int id;
    char name[50];
    struct node *next;
}*head;

You should only define the struct:

Code:

struct node{
    int id;
    char name[50];
    struct node *next;
};

Then declare head to be local to a function, e.g., the main function.

Anyway, your problem is that you declared a pointer to a node, but you did not actually allocate memory for that node, e.g., by calling malloc. I suggest that you write a few functions for manipulating a singly linked list, e.g., write a function to add a node to the head of the linked list, write a function to destroy the linked list, write a function to print the linked list. You might also find it convenient to define another struct for the linked list itself, to hold not only the head pointer, but also a tail pointer.

Thanks for the prompt response, yea that fixed it, wondering how I missed that.

As for the bad practice, you should tell that to my CS 1 teacher, who claims to be doing this for over 15 years.

The program is working, but I'm still unable to print my LinkedList, it says that it's empty.

The program is still a work in progress as I'm trying to build it off my teachers code:

Code:

#include<stdio.h>
#include<stdlib.h>
#include<string.h>
#define LINESIZE 128


struct node
{
    int id;
    char name[50];
    struct node *next;
}*head;


// Function prototypes
void insert(char *t_name, int *t_id);
void display(struct node *r);
int deletebyid(int num);
int deletebyname(char name);


// main function
int  main()
{
    int i, t_id, num, val;
    struct node *temp;
    temp = (struct node*)malloc(sizeof(struct node));
    char name[50], t_name[50], input_name[50], line[LINESIZE], *value;
    struct node *head = NULL;
    FILE * ifp = fopen("AssignmentOneInput.txt", "r");




    while ((fgets(line, sizeof(line), ifp)))
        {
            value = strtok(line, ",");
            num = strtok(NULL, "\n");
            val = atoi(num);
            temp->id = val;
            strcpy(temp->name,value);
        }




    while(1)
    {
        printf("\nList Operations\n");
        printf("===============\n");
        printf("1.Insert\n");
        printf("2.Display\n");
        printf("3.Delete by ID\n");
        printf("4.Delete by name\n");
        printf("5.Exit\n");
        printf("Enter your choice : ");


        if(scanf("%d", &i) <= 0)
        {
            printf("Enter only an Integer\n");
            exit(0);
        }
        else
        {


            switch(i)
            {


                case 1:
                    printf("Enter the name to insert : ");
                    scanf("%s", t_name);
                    printf("What is the id number for %s?\n");
                    scanf("%d", &t_id);
                    insert(t_name, t_id);
                    break;


                case 2:
                    if(head == NULL)
                    {
                        printf("List is Empty\n");
                    }
                    else
                    {
                        printf(" %s\,%d\n", temp->name, temp->id);
                    }
                    break;


                case 3:
                    if(head == NULL)
                    {
                        printf("List is Empty\n");
                    }


                    else
                    {
                        printf("Enter the ID number to delete : ");
                        scanf("%d",&num);
                    }


                    if(deletebyid(num))
                    {
                        printf("%d deleted successfully\n",num);
                    }


                    else
                    {
                        printf("%d not found in the list\n",num);
                    }
                    break;


                case 4:
                    if(head == NULL)
                    {
                        printf("List is Empty\n");
                    }


                    else
                    {
                        printf("Enter the name to delete : ");
                        scanf("%s", name);
                    }


                    if(deletebyname(input_name))
                    {
                        printf("%s deleted successfully\n",name);
                    }


                    else
                    {
                        printf("%s not found in the list\n",name);
                    }
                    break;


                case 5:
                    return 0;


                default:
                    printf("Invalid option\n");
            }
        }
    }


    fclose(ifp);
    return 0;
}


void insert(char *t_name, int *t_id)
{
    struct node *temp;


    temp = (struct node*)malloc(sizeof(struct node));


    strcpy(temp->name,t_name);
    temp->id = t_id;
    temp->next = NULL;


}


void display(struct node *r)
{
    r = (struct node*)malloc(sizeof(struct node));


    r = head;


    if(r == NULL)
    {
        return;
    }


    while(r != NULL)
    {
        printf(" %s\,%d\n", r->name, r->id);


        r = r->next;


        if(r == NULL)
        {
            printf("found the end of our linked list!");


        }
    }


    printf("\n");
}


int deletebyid(int num)
{
    struct node *temp, *prev;
    temp = head;


    temp = (struct node*)malloc(sizeof(struct node));
    prev = (struct node*)malloc(sizeof(struct node));


    while(temp != NULL)
    {
        if(temp->id == num)
        {
            if(temp == head)
            {
                head = temp->next;
                free(temp);
                return 1;
            }
            else
            {
                prev->next = temp->next;
                free(temp);
                return 1;
            }
        }


        else
        {
            prev = temp;
            temp = temp->next;
        }
    }
    return 0;
}


int deletebyname(char input_name)
{
    struct node *temp, *prev;
    temp = head;


    temp = (struct node*)malloc(sizeof(struct node));
    prev = (struct node*)malloc(sizeof(struct node));


    while(temp != NULL)
    {
        if(temp->name == input_name)
        {
            if(temp == head)
            {
                head = temp->next;
                free(temp);
                return 1;
            }


            else
            {
                prev->next = temp->next;
                free(temp);
                return 1;
            }
        }


        else
        {
            prev = temp;
            temp = temp->next;
        }
    }
    return 0;
}

Originally Posted by laserlight

Your teacher could be justified in showing the syntax for convenience as an example with the limited screen space in class, but in practice you should not do it for your own code. The reason is that you end up declaring head as a global variable. Global variables introduce global state, which is usually a Bad Thing as it makes it harder for you to reason about your code, thus your code will be harder to debug and maintain.


Start by changing your program to treat head as a local variable. For example, instead of:

Code:

void insert(char *t_name, int *t_id);

it could be:

Code:

void insert(struct node *head, char *t_name, int *t_id);

I really doubt it to be the white space as she also has them in her powerpoints and wants us to learn it that way.



So I've added head as a local variable, I'm thinking that I need an add function so that I can call it to use inside the insert function, what do you think?

Originally Posted by laserlight

Your display function should not be calling malloc.

Yea that makes sense. How should I call that struct or what would you suggest?

So even when I try to display it from the switch statement, I don't get any output. I feel like my data isn't being loaded into the linklist at all....

If you don't use a global pointer for the head of your list, you have to pass the head of your list to your functions.

Except there's a problem. Part of inserting into the list will require that you change where the head pointer points to, in-order to insert efficiently. Using a global pointer you can do this easily because your function can just access the global pointer and change where it points to no problem, the global is at file scope so its visible to everything.

Using a global however, makes it so you can only have ONE list at any given time, that is horrible. You also invade global namespace and create other problems as mentioned by laserlight.
So you still need to pass your pointer around, but you need to somehow be able to change where it points to.

That means you need to declare your insert function like this:

Code:

void insert(struct node ** head, ...);

Now we can change the value of the head pointer because we passed the address of it to the insert function. This creates additional syntatic woes that really aren't a big deal for seasoned programmings but can be burdensome to debug none-the-less.

So what's a better way? Simple, use a structure to store the head (or better yet, the tail and have it point to the head *read about circular linked lists to find out more*).

Code:

struct node { ... };
struct list { struct node * head };

/* Now we can declare insert like this */
void insert(struct list * list, ...) {
   ...
   list->head = new_head; /* Change what head pointer pointers to */
   ...
}

Thanks for the response buddy, but I'm having a little hard time trying to understand that even if I localize my head pointer to every function, I'm still having issues trying to use that pointer in my program.