0
struct sdata;
typedef struct sdata *sptr;

struct sdata {
char sName[25];
char sNum[7];

sptr next;
};

void readdata();
int main(void)
{
readdata();
return 0;'
}

void readdata(){
     int length;
     int i ;
    char Line [100]; 
    const char s[2] ="#";
    char *token= strtok(Line,s);
    FILE * data =fopen("data.txt","r") ;

    if(data==NULL){
        printf("ERROR : The File Cannot Be Found");
    }else{
        /*Read The Student File Line By Line */
    while(fgets(Line,100,Student_info)!=NULL ){
        printf("-----> %s\n",Line);
        sptr stemp = (sptr)malloc(sizeof(struct data));
        stemp->next=NULL;

        token = strtok(Line,s);
        strcpy(stemp->sName, token);

        token = strtok(NULL,s);
        strcpy(stemp->sNum, token);

        while(token !=NULL){
            token = strtok(NULL,s);
            length=strlen(token);
            if(token[length-1]=='\n')
                token[length-1]='\0';
}

 stemp=stemp->next;
    }
    }
}
2
Contributors
1
Reply
28
Views
4 Months
Discussion Span
Last Post by Nutster
0

There were several problems with your code. I had to make some significant changes just to get it to compile.

  1. I added #include lines at the top to read in the required function declarations.
  2. You had two different names for your file handle. Student_Info was a better name than just data, so I kept that one and changed data to match.
  3. You used the wrong name in malloc. The inner argument needed to be struct sdata; you wrote struct data, which was not defined.
  4. I removed extranous characters. (e.g. See the end of your line 15)

I refactored your code, making the following adjustements to make it easier to read, maintain and less vulnerable to problems.

  1. I added blank lines to make things easier to read. You generally want to put a blank line after your variable declarations, before the rest of your function body.
  2. I removed some blank lines that were visually breaking up the flow of your program, such as the blank line between the data members of your struct and the pointer to the next node.
  3. I adjusted the indentation of the code to make it easier to read. Well, my IDE did most of the work, but I helped. The curly braces around blocks of code should start on their own line.
  4. I moved the main function to the end, so that the function definition for readdata could also act as the declaration; you did not need a seperate declaration line for it.
  5. The readdata function looks like it is supposed to read the data then do something with it. You have it returning nothing: void. I changed it to return the linked list it was building. More about that as we go on.
  6. I decided to pass the filename to the readdata function to increase flexibility. I also checked to make sure it was not a NULL pointer as one of the first actions I took in the function.
  7. I removed the initialization for token as it did not make sense there as Line did not have anything in it yet.
  8. I removed const char s[2] = "#";. Normally I like putting constants before the variables, but that was not main problem. This constant does not add to readability later on. Maybe a better name would help, but in this case, I decided to just put the string in the function calls.
  9. I moved the declaration of stemp out of the loop. There is no need to recreate the variable each time through the loop.
  10. I added sRoot to point to the first node of the linked list. I added sAdding to keep track of where the next element of the linked list gets added.
  11. I removed i from the declaration list as it is not used in the function.
  12. If the fopen call fails, then I not only print the error message, but also return an empty linked list. I added the filename to the error message.
  13. I checked the end-of-file as the loop condition and moved the fgets call inside the loop. If the fgets call fails, it still gets out of the loop by using break;.
  14. I simplified the line output message by removing the leading arrow.
  15. Always make sure the return value from malloc is not NULL. If you run out of memory, your program has just begun to fail. In that circumstance, I close the file and return the linked list that has already been read.
  16. Always make sure that a pointer is not null before following it with * or ->.
  17. I changed the delimiter to strtok to "#\n". This way, not only does strtok look for the hash marks between fields, but also the new line at the end of the string. This also makes it easier to manage malformed lines by looking for new line at the end of the read string.
  18. Check if strtok returned NULL, meaning it could not find the given delimiters. In that case, you have a bad text line. I free the pointer to prevent a memory leak and restart the loop.
  19. If strtok returns a non-NULL pointer, it will copy the string into the node. I have used strncpy instead of strcpy to prevent string overflow.
  20. If strncpy reaches the limit of characters, it does not put in a null character at the end of the string, so I added a line to put a null characters at the end of the array. If the Name is shorter than 25 characters, putting a null character after the shorter string will do nothing to it. If the Name is 26 characters or longer, it will put the null character at the end of the string, just a C expects.
  21. The loop at the end is not needed. You do not have to go through the rest of the string to get ready to read the next line. When you give strtok a new string, it resets, completely forgetting about the last string. Also, I removed length from the variable declarations as it no longer needed.
  22. I am not sure what stemp = stemp->next; is supposed to do, but what it is doing is creating a memory leak by throwing away the the node you just created without using the free function. I have added code to actually add the new node to the linked list instead.
  23. At the end, I return the linked list so that main can do something with it, in this case printing the data that was successfully read by walking through the linked list.

My modified code:

/* these are needed to declare the appropriate functions */
#include <stdio.h>  /* fopen, fgets, fclose, printf */
#include <string.h> /* strtok */
#include <stdlib.h> /* malloc, free, NULL */

struct sdata;

typedef struct sdata *sptr;

struct sdata {
    char sName[25];
    char sNum[7];
    sptr next;
};

sptr readdata(char *filename) /* Return the linked list you built.  Passing the filename increases flexibility. */
{
    char Line[100];
    char *token; /* Initialization was premature */
    FILE *Student_Info; /* use a consistent variable name */
    sptr stemp; /* no need to recreate this every time through the loop */
    sptr sRoot = NULL; /* the start of the linked list.  Starts with nothing in it. */
    sptr sAdding = NULL; /* The last node in the linked list.  This will be where new nodes get added to the list. */

    /* make sure the filename was given properly */
    if (filename == NULL)
        return NULL; /* nothing to do, so go back to caller */
    Student_Info = fopen(filename, "r");
    if (Student_Info == NULL) 
    {
        printf("ERROR: The file %s cannot be found.", filename);
        return NULL;
    }
    /* Read The Student File Line By Line */
    /* Check for end-of-file at the start of the loop. */
    while (!feof(Student_Info)) 
    {
        if (fgets(Line, 100, Student_Info) == NULL)
            /* if we do not get a valid line, break the loop */
            break;
        printf("%s\n", Line);
        stemp = (sptr)malloc(sizeof(struct sdata)); /* used name of structure, not FILE */
        if (stemp == NULL) /* Added check:  If malloc fails (out of memory), abort */
        {
            fclose(Student_Info);
            return sRoot;
        }

        stemp->next = NULL;

        /* Look for a # or a new line. */
        token = strtok(Line, "#\n");
        if (token == NULL) /* Added check: if it can not find the token */
        {
            /* release the memory for the linked list node */
            free(stemp);
            /* go back and try reading the next line */
            continue;
        }
        else
        {
            /* I am using strNcpy to control how many characters to copy.*/
            strncpy(stemp->sName, token, 25);
            /* put a null character at the end of the string, just in case we read too long a string.  In that case, strncpy does not append the null at the end of the string. */
            stemp->sName[24] = '\0';
        }

        token = strtok(NULL, "#\n");
        if (token == NULL) /* Check to see if there is no second token.  In that case, free this structure, abandon the line and try again. */
        {
            /* release the memory for the linked list node */
            free(stemp);
            /* go back and try reading the next line */
            continue;
        }
        else
        {
            /* I am using strNcpy to control how many characters to copy.*/
            strncpy(stemp->sNum, token, 7);
            /* put a null character at the end of the string, just in case we read too long a string.  In that case, strncpy does not append the null at the end of the string. */
            stemp->sNum[6] = '\0';
        }

        /* Add to linked list */
        if (sRoot == NULL) {
            /* if the root of the linked list is NULL, start the linked list with the current node. */
            sRoot = stemp;
            sAdding = sRoot;
        }
        else
        {
            /* Otherwise, add the current node at the end of the linked list. */
            sAdding->next = stemp;
            /* Move the adding point to the new node */
            sAdding = stemp;
        }
    }
    /* close the file when done reading. */
    fclose(Student_Info);
    /* Return the linked list that was just read. */
    return sRoot;
}

int main(void) /* relocated to reduce clutter */
{
    sptr sList;
    sptr sWalker;

    sList=readdata("data.txt");
    /* do what you want with list. */
    /* like, print it!*/
    for (sWalker = sList; sWalker != NULL; sWalker = sWalker->next)
        printf("%s: %s\n", sWalker->sNum, sWalker->sName);

    return 0;
}

Edited by Nutster: Fix wording.

Have something to contribute to this discussion? Please be thoughtful, detailed and courteous, and be sure to adhere to our posting rules.