Re: [C] [newbie] problem reading from file

From: Francis Glassborow (francis_at_robinton.demon.co.uk)
Date: 03/12/04 

Date: Fri, 12 Mar 2004 11:43:11 +0000

In message <4050C96A.9060203@bogus.net>, Curley Q. <curleyq@bogus.net>
writes
>I want to read numbers from a text file. In the final form of the
>program the numeric strings will be converted to integers and used in
>calculations.
>
>/* read numbers from a text file */
>#include <stdio.h>
>#include <stdlib.h>
>#include <string.h>
>
>int main(int argc, char *argv[])
>{
> FILE *fp;
> char *filename, ch;
why did you declare two variables in one statement here? Where all the
variables are the same type that is acceptable but mixing pointer types
with plain types makes it a poor idea.

> char *number;
Why are you using a pointer? Even in C you can delay declarations to the
start of a block. Leave that line out [or make it a suitable array]
> char n[2];
likewise, drop that declaration.
> int t;
and this one
>
> if(argc != 2)exit(EXIT_FAILURE);
don't just exit, print a message saying why the program aborted.
> filename = argv[1];
> fp = fopen(filename, "r");
check the file opened
> while(ch != EOF)
first time this is used ch is unitialised, and then there is the small
problem that EOF will not be a char value. If you check getc() you will
note that it returns an int, not a char. Change ch to an int, and insert
   ch = getc(fp);
immediately before you enter this loop.
> {

> number = calloc(8, sizeof(char));
     replace with:
     char number[8] = {'\0'};
> while((ch = getc(fp))!= ' ')
OK once you have fixed the fact that ch was the wrong type and
uninialised. However note that it assumes that the file only contains
digits and spaces. check out the isdigit() and isspace() library
functions and use them to validate input.
> {
> t = ch - '0';
So that converts the numerical code to the equivalent numerical value
> sprintf(n, "%d", t);
but this effectively reverses the process.
replace the lines with:
       char n[2] = "";
       n[0] = ch;
> strcat(number, n);
actually I would consider converting all this inner loop to a for-loop
that places each input character into number as long as there is still
space (i.e. less than eight characters have been used)
> }
> printf("%s\n", number);
of course all your code could have been replaced by using fscanf() and
printf:-)
> free(number);
and this line is not necessary if you avoid using an unnecessary dynamic
array.
> }
> fclose(fp);
>
> return 0;
>}

I hope you do not feel bad because I have commented almost every line.
It was because your code clearly expressed what you were trying to do
that I could pick it to pieces and that puts it a long way ahead of
confused muddled code which cannot be critiqued this way. 

-- 
Francis Glassborow      ACCU
Author of 'You Can Do It!' see http://www.spellen.org/youcandoit
For project ideas and contributions: http://www.spellen.org/youcandoit/projects

Relevant Pages