Re: fgets problem

Alan Peake <adpeake@xxxxxxxxxxxxxxxxxxxxxxxxx> writes:
I've been trying to read a text file into an array without much
success. The following is part of "main" and all appears OK until the
statement while (fgets(str,20,fp) !=NULL)
Sometimes it works fine and the file xtl.dat (argv[1]) is read in
perfectly and the program performs as expected. But more often than
not, it appears to treat the first line of xtl.dat (which is "0.51400)
as NULL and skips the rest of the file.
Can anyone see what I've done wrong?
Thanks
Alan

#include "stdio.h"
#include <math.h>
#include <stdlib.h>

float f[140] ;

main(argc,argv)
int argc ;
char *argv[] ;
{
FILE *fopen() , *fp ;
float freq=20.0,tol=0.0005 ;
float freqin(),tolin() ;
char *str,c ;
int i=0,j,k,size=120 ;
if ((fp=fopen(argv[1],"r")) == NULL) {
printf("No crystal list file %s\n",argv[1]) ; exit(1) ;
}
while (fgets(str,20,fp) != NULL) {
f[i]=atof(str) ;
for (i=0;i<130;i++) {
fgets(str,20,fp) ;
f[i] = atof(str) ;
printf(" %10s %3.6f %d",str,f[i],i) ;
}
fclose(fp) ;

The code you posted is incomplete; you're missing closing braces for
the while loop and for the main function. Judging by the indentation,
you call fopen() exactly once, but you call fclose() on each iteration
of the while loop. Calling fgets() on a closed file invokes undefined
behavior. I suspect that's the cause of your problem, but it's hard
to be sure without seeing your input.

There are a number of other things you should fix; some of them are
errors, some are style issues.

Include directives for standard headers should use <>, not "".
Change
#include "stdio.h"
to
#include <stdio.h>

You don't use anything from <math.h>, so that header isn't necessary.
This might be an artifact of the way you've trimmed the program for
posting (thank you for that).

Your array "f" is used only inside main, so there's no reason to
declare it at file scope. Again, this might be an artifact of the way
you've trimmed the program for posting, but it might still make more
sense to declare it in one function and pass a pointer to other
functions that use it.

I personally dislike putting a space in front of each semicolon, and I
always put a space after each comma. Not all C programmers share this
particular opinion, but I think most do.

Your declaration of main is an old-style declaration; there's no
longer any good reason to use such declarations. Use:
int main(int argc, char *argv[])
(I personally prefer "char **argv", but "char *argv[]" is certainly
valid.)

There's no need to re-declare fopen, and in fact it's a bad idea to do
so; <stdio.h> does that for you.

I find code that declares one thing per line to be clearer:
float freq = 20.0;
float tol = 0.0005;

It usually makes more sense to use double rather than float; it
usually provides more precision and range, and on many CPUs it's just
as fast.

You declare freqin() and tolin(), but you don't define or use them.
If they exist in your actual code, it would be better to provide
prototypes at file scope, probably in a header file. It's legal to
have function declarations inside a function definition, but it's
generally a bad idea.

exit(1) is, strictly speaking, not portable; use exit(EXIT_FAILURE)
unless you're sure that you specifically want to pass a status of 1 to
the calling environment.

130 is a "magic number"; there's nothing in your program that explains
why it has that value. Declare a constant, perhaps using #define.

In the loop, you call fgets() without checking the result. This is a
bad idea; you have no way of detecting failure. This makes your
program "brittle" in the presence of bad input data. You should also
think about what to do if an input line is longer than what your array
can hold.

Don't use the atof() function; it doesn't detect errors, and in fact
can invoke undefined behavior in some cases. (The exact circumstances
in which the behavior is undefined are not entirely clear, which is
another good reason not to use it.) Use strtod() instead; it's a
little more complicated, but the extra safety is well worth i.

All your output, other than the first line, is on a single line with
no '\n' characters. Was that your intent? Strictly speaking, if your
implementation requires a terminating '\n' at the end of the last line
of output, then failing to provide one can invoke undefined behavior
(though on most systems you'll just get a long line with no
end-of-line marker at the end) Perhaps you wanted:
printf(" %10s %3.6f %d\n", str, f[i], i);

--
Keith Thompson (The_Other_Keith) kst-u@xxxxxxx <http://www.ghoti.net/~kst>
Nokia
"We must do something. This is something. Therefore, we must do this."
-- Antony Jay and Jonathan Lynn, "Yes Minister"
.

Relevant Pages

  • Re: sound synthesis
    ... > the declarations and using short float helped, ... (defun mix (target-samples source-samples start sample-rate) ... (declare (float sample-rate seconds) ... (defun fm-gong (time freq) ...
    (comp.lang.lisp)
  • Re: Extending T-SQL with COM
    ... Using Excel for this is an extremely heavy weight way of performing what ... declare @rate float ... > GRANT EXECUTE ON dbo.sp_hexadecimal TO Public ...
    (microsoft.public.sqlserver.programming)
  • Re: Zip code radius search
    ... Here is an example showing the formula for the distance between two ... declare @lat1 as float ... declare @lat2 as float ...
    (microsoft.public.sqlserver.programming)
  • Re: Problem with OPENXML
    ... Use of included script samples are subject to the terms specified at ... EXEC sp_xml_preparedocument @idoc OUTPUT, @doc, '<gpx ... WITH (lat float, lon float, ele float) ... DECLARE @doc varchar ...
    (microsoft.public.sqlserver.xml)
  • Re: should this work
    ... >> work when i was compiling it i was getting warning messages but then ... The reason i wrote the program was to get ... >Why declare these as globals? ... Why would you expect a warning when the conversion from char to float ...
    (comp.lang.c)