0

I have just started working with Linked Lists and am not quite sure if I'm doing it correctly. I am trying to initialize a linked list and fill it with information from a .txt file. Then with the print function, I just want to print out the info in my linked list. However It crashes every time I try to traverse through the linked list and print out what's in the pointer. Any tips would be helpful and appreciated. Here's my code:

struct employeeData {
   int EMP_ID;
   char name[20];
   int dept;
   int rank;
   double salary;
   };

struct employeeData employee;

struct node {
   struct employeeData employee;
   struct node *next;
   };

struct node *myList = NULL;

struct node initializeList (struct employeeData employee[], struct node myList[]);
void print (struct employeeData employee[], struct node myList[]);

int main () {
    int x;

    initializeList(&employee, &*myList);
    print(&employee, &*myList);

    System("PAUSE");
    return 0;
}

struct node initializeList (struct employeeData employee[], struct node myList[]){
         struct node *newNode = (struct node*)(malloc(sizeof(struct node)));

 FILE *ifp;

 ifp = fopen("empInfo.txt", "r");

 fscanf(ifp, "%d %s %d %d %lf\n", &newNode->employee.EMP_ID, newNode->employee.name, &newNode->employee.dept, &newNode->employee.rank, &newNode->employee.salary);
 //newNode->next = NULL;
 myList = newNode;
 struct node *temptr = myList;

 while (newNode->employee.EMP_ID != 0) {

       fscanf(ifp, "%d %s %d %d %lf\n", &newNode->employee.EMP_ID, newNode->employee.name, &newNode->employee.dept, &newNode->employee.rank, &newNode->employee.salary);

       temptr->next = newNode; 
       newNode->next = NULL;   
       } 

 return *myList;
}

void print (struct employeeData employee[], struct node myList[]){

         struct node *temptr = myList;
                           printf("WOW");

 while(temptr->next!=NULL){
                           printf("WOW");
          printf("%d %s %d %d %lf\n", temptr->employee.EMP_ID, temptr->employee.name, temptr->employee.dept, temptr->employee.rank, temptr->employee.salary);
          temptr = temptr->next;
          }
}
Improve this question
1
  • Note that when you're debugging, it is best to ensure that debugging outputs appear by at least ending each printf() with a newline. You might even need to add a fflush(stdout); too. Otherwise, output is not guaranteed to appear in a timely fashion. (Even with newlines, if the output is piped to a command such as less, the newlines may not force the data out to the pipe.) Commented Sep 21, 2014 at 22:25

2 Answers 2

Reset to default
2

First of all, you don't have to create two seperate structs to make a linked list ..
You can do it like this:

struct list {
    /* struct members */
    struct list *next;
}

Just like that ! Also, In functions you should be aware of pointer decay .. In simple terms, it's when you pass a function an array, the function recieves it as a pointer (which is technically different). Best way to deal with this is to receive an array as struct employeeData ** data_array, and a pointer as struct employeeData * data. So there is no meaning in struct employeeData employee[]. Also, this: &*employee is exactly the same as this: employee, except that the second is slightly more efficient, because in the first one you get the address of the variable and then dereference it, essentially doing nothing.

In the structs definitions ..

There's no need to define a struct called node, because that just complicates the program and confuses you. The way linked lists are actually implemented is by adding a member of the same type as the struct its defined in, as I explained eariler.

Here is the improved version:

struct employeeData {
   int EMP_ID;
   char name[20];
   int dept;
   int rank;
   double salary;
   struct employeeData *next;
};

In the initializeList function ..

There's no need for the second argument if you're just going to duplicate it in the function .. Also, you don't need to derefence the malloc'd pointer before returning it back, because the function caller won't probably be able to deduce that he needs to free it afterwards to prevent a memory leak, unless he uses a tool like valgrind ..

You also don't need to invoke fscanf twice. I also renamed the function to be: getEmployeeData, because I think that makes more sense.

This is the final form of the improved function:

struct employeeData *getEmployeeData (const char * filename) {
    FILE *ifp = fopen(filename, "r");
    if ( ifp == NULL )
        return NULL;

    struct employeeData *employee = malloc(sizeof(struct employeeData));
    struct employeeData *temptr = employee;

    int num = (int)getNumberOfLines(filename);

    for (int line = 1;
        (fscanf(ifp, "%d %s %d %d %lf\n", &temptr->EMP_ID, temptr->name, &temptr->dept, &temptr->rank, &temptr->salary) == 5)
            && line < num; ++line) {

        temptr->next = malloc(sizeof(struct employeeData));
        temptr = temptr->next;
    }

    fclose(ifp); /* fopen uses malloc internally */
    return employee;
}

You might notice that (unlike your version of the function) I do this: temptr->next = malloc(sizeof(struct employeeData)). This is most certainly the reason why your program crashed, it's because you only malloc the first element of the node and try to use fscanf on struct members that aren't even allocated in memory. And that's why you have to allocate it before you use it. Remember, each element in a node is (mostly) independent from the other, even in memory allocation.

This function is simpler and much more efficient. You might also notice that I call another function called getNumberOfLines that gets the number of lines in a file.

Here it is:

size_t getNumberOfLines(const char * filename) {
    FILE *stream = fopen(filename, "r");
    if ( stream == NULL )
        return EOF;

    size_t lines = 0;
    char c;
    while (( c = getc(stream))  != EOF )
        if ( c == '\n' )
            lines++;
    fclose(stream);
    return lines;
}

The reason I did this, is because if fscanf doesn't find the formatted text to store in the variables, it simply stores "0", as a float, an int, a char or even a string. So, if fscanf scans an empty line, it simply stores 0 in all of the variables ..

To prevent that, you have to make sure that fscanf scans only occupied lines, even if they aren't correctly formatted, because the other condition that checks if fscanf returned 5 (the number of variables required to store in) will not be true if the line isn't correctly formatted but will however return true if the line isn't even occupied ( This is what I experienced with the gcc implementation, if you don't need that, remove it ).

The print function

void print (struct employeeData *employee){
    for( struct employeeData *temptr = employee; ( temptr != NULL ); temptr = temptr->next )
        printf("%d %s %d %d %lf\n", temptr->EMP_ID, temptr->name, temptr->dept, temptr->rank, temptr->salary);
}

I think I explained all the ideas applied here. Let's move on ..

Memory freeing

We need to free dynamic memory to prevent memory leaks, and when you're trying to free a linked list that becomes even trickier! If you try to free them sequentially that most certainly won't work, unless some universal coincidence happens at the time your program was running! The reason for that is simply really .. It's because the only way you can link to the next list is through a member of the struct at hand. Obviously, if you delete all your struct's members from memory, then you have no clue where to look for the next list! One solution to this is through recursion, like this:

void releaseData(struct employeeData *data) {

    /* freeing nodes in reverse because nodes are connected in memory, if you free the first, you lose grasp on the second and the third, resulting in a memory leak .. */
    if (data->next)
        releaseData2(data->next);
    free(data);
}

But I don't prefer this method, because it triggers allocating memory for functions and their arguments then deallocating them, and also keeping track of the calling function, and there's actually a limit to that depending entirely on the operating system and the running kernal to determine. So as you can see, this method is mostly avoidable and is only used when there's no other way, and that's why I've created this one:

void releaseData(struct employeeData *data) {

    /* freeing nodes in reverse because nodes are connected in memory, if you free the first, you lose grasp on the second and the third, resulting in a memory leak .. */

    struct employeeData * temptr = data;
    int num = 0, first = 1;

    while ( temptr != NULL ) {
        if ( temptr->next != NULL ) {
            if (first) {
                while ( temptr->next != NULL ) {
                    temptr = temptr->next;
                    num++;
                }
                first = 0;
            } else {
                for(int i = 0; i < num - 1; ++i)
                    temptr = temptr->next;
                num--;
            }
        }
        free(temptr);
        temptr = (num == 0) ? NULL : data;
    }

    /* We could have used recursion, but that come with unnecessary overhead and less memory efficiency */

}

As you can see, this one is much more complex but it is also much more effiecient.
We use two variables to keep track of the loop: num and first.

num is used to count how many nested nodes are there to go through, because when I free a pointer, it most certainly is not NULL so the loop would be infinite because it simply checks for a value in there ..

first is used to indicate if it is the first time to run the loop, because if it was, we most certainly wouldn't know how many nodes are in there.

I think the rest of the function is self-explanatory so I'd leave it to you to figure it out.

The main function

int main () {
    /* Never dereference a malloc'd pointer before freeing it, so we'll get the pointer returned from the function as is */
    struct employeeData *employee = getEmployeeData("empInfo.txt"); /* Getting employee data in the struct */
    print(employee); /* Printing them out */
    releaseData(employee); /* Freeing Dynamic Memory */
    return 0;
}

That's it.

Improve this answer
Sign up to request clarification or add additional context in comments.

10 Comments

Thanks! This is really helpful! One question, if I do keep it in two structs like I have for the list, am I passing the right parameters to my print function?
@user3281576: If you really want to use two structs for such a simple functionality like a linked list then I advise that you keep a pointer to the other struct (employeeData in this case) instead of requiring a copy like you did in your initial version of the node struct. Creating a wrapper node struct is ofcourse useful if you want to make a linked list out of a struct that you don't have defined in your code, like the FILE struct (if you want to link between files).
As for your question about passing parameters, you should only pass the node struct as it acts as a wrapper above the other one.
I ate to sound stubborn it's just the guidelines say that I have to have two structs. I think I updated my two functions correctly, but I have not put in the freeMemory function. The program reads in the data to the linked list correctly I think because if I print the data out in the Initilize function it prints correctly, but when I try to print it in the print function it doesn't work and crashes. Also, If I put the print statements in the for loop you made, it doesn't even run the for loop once.
Here's the print function: void print (struct node *myList) { for( struct node *temptr = myList; ( temptr != NULL ); temptr = temptr->next ){ printf("WOW"); printf("%d\t%s\t%d\t%d\t%lf\n", temptr->employee.EMP_ID, temptr->employee.name, temptr->employee.dept, temptr->employee.rank, temptr->employee.salary); }
|
1

Your code of course is wrong. I would write separate function add that adds a new node to the list and separate function initialize that uses the function add and appends data from a file to the list.

Take into account that function add can be written such a way that either it will add a new node at the beginning of the list or at the end of the list. If you fill the list with data from a file then it is better to write function add that it would add a new node at the end of the list.

I can show a template that you can use for your assignment. You have to write function initialize yourself and make minor changes to function add that it would assign values for all fields of data member employee of a node.

Here is the template

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

struct employeeData 
{
    int EMP_ID;
    char name[20];
    int dept;
    int rank;
    double salary;
};

struct node 
{
    struct employeeData employee;
    struct node *next;
};

struct node ** add( struct node **list, struct employeeData employee )
{
    struct node *new_node = malloc( sizeof( struct node ) );

    new_node->employee = employee;
    new_node->next = NULL;

    if ( *list == NULL )
    {
        *list = new_node;
        return list;
    }
    else
    {
        ( *list )->next = new_node;
        return &( *list )->next;
    }
}

void clear( struct node **list )
{
    struct node *current = *list;

    while ( current != NULL )
    {
        struct node *tmp = current;
        current = current->next;
        free( tmp );
    }

    *list = NULL;
}

void print( struct node *list )
{
    for ( ; list != NULL; list = list->next )
    {
        printf( "%d\n", list->employee.EMP_ID );
    }
}

int main( void )
{
    struct node *list = NULL;
    struct node **last = &list;
    int i;

    for ( i = 0; i < 10; i++ )
    {
        struct employeeData employee = { i };
        last = add( last, employee );
    }

    print( list );

    clear( &list );

    assert( list == NULL );

    return 0;
}

The output is

0
1
2
3
4
5
6
7
8
9

Function initialize could be declared as

initialize( struct node **list, const char filename[] );
Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.