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;
}
}
}
Nutster 58 Newbie Poster
There were several problems with your code. I had to make some significant changes just to get it to compile.
- I added
#include
lines at the top to read in the required function declarations. - You had two different names for your file handle.
Student_Info
was a better name than justdata
, so I kept that one and changeddata
to match. - You used the wrong name in
malloc
. The inner argument needed to bestruct sdata
; you wrotestruct data
, which was not defined. - 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.
- 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.
- 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.
- 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.
- I moved the
main
function to the end, so that the function definition forreaddata
could also act as the declaration; you did not need a seperate declaration line for it. - 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. - 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. - I removed the initialization for
token
as it did not make sense there as Line did not have anything in it yet. - 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. - I moved the declaration of
stemp
out of the loop. There is no need to recreate the variable each time through the loop. - I added
sRoot
to point to the first node of the linked list. I addedsAdding
to keep track of where the next element of the linked list gets added. - I removed
i
from the declaration list as it is not used in the function. - 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. - I checked the end-of-file as the loop condition and moved the
fgets
call inside the loop. If thefgets
call fails, it still gets out of the loop by usingbreak;
. - I simplified the line output message by removing the leading arrow.
- 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. - Always make sure that a pointer is not null before following it with
*
or->
. - I changed the delimiter to
strtok
to "#\n". This way, not only doesstrtok
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. - Check if
strtok
returned NULL, meaning it could not find the given delimiters. In that case, you have a bad text line. Ifree
the pointer to prevent a memory leak and restart the loop. - If
strtok
returns a non-NULL pointer, it will copy the string into the node. I have usedstrncpy
instead ofstrcpy
to prevent string overflow. - 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. - 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 removedlength
from the variable declarations as it no longer needed. - 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 thefree
function. I have added code to actually add the new node to the linked list instead. - 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 because: Fix wording.
Be a part of the DaniWeb community
We're a friendly, industry-focused community of developers, IT pros, digital marketers, and technology enthusiasts meeting, networking, learning, and sharing knowledge.