You have written everything in one function (main
probably) but it is better
split into its components. Essentially what you want to do is:
open input file
get column count
get line count
rewind file
allocate memory for the array
read array data
Some of these stages could and probably should be in separate functions.
Taking it from the top, you open the input file, with the file name
hard-coded. A better approach is to get the filename from the command line -
argv[1]
. You have checked for failure to open the file, which is good.
The normal error handling is to use perror
and the name of the failed file:
FILE *f = fopen(argv[1], "r");
if (!f) {
perror(argv[1]);
return EXIT_FAILURE;
}
You then allocate buffer data
(again the error handling should use
perror
). You then use this buffer in a read loop to count the entries in
each line, but you really don't need to save the values in the buffer at all
at this stage - you discard them anyway. Your read loop could equally be:
while (sscanf(line+cur, "%*lf%n", &read) == 1) {
cur += read;
columCount++;
}
Notice that the format string uses a '*' to suppress assignment of the
floating point value read. Note also the improved formatting and the use of
++
in columCount++
. Note also that you don't check for failure in reading
the first line.
Edit: Abbas pointed out that adding the '*' to suppress assignment means that we must check for 0 returned from
sscanf
, not 1. But
sscanf
also returns 0 when it fails to read anything (in this case the read count in
read
would also contain 0). Hence a bad character in the line will cause the loop to hang. So we must either not suppress the assignment:
double d;
while (sscanf(line+cur, "%lf%n", &d, &read) == 1) {
cur += read;
columCount++;
}
or we must check read
.
while ((sscanf(line+cur, "%*lf%n", &read) == 0) && (read != 0)) {
cur += read;
columCount++;
}
I think I would go for the first. End of edit.
You then count the lines by looping through them. Note that you use a fixed
size buffer line
, allocated at the beginning, to read each line. To be
completely flexible, you should handle lines of arbitrary length. Your lines
are fixed at 1000 chars and anything beyond that breaks the code. Since you
are using getline
(which is non-standard, but useful) I would make use of its
ability to allocate a line for you by passing a zero size.
char *line = NULL;
size_t count = 0; // EDIT: Abbas pointed out we need to add count=0
getline(&line, &count, file);
// use the line
free(line);
In this, line
is just a pointer. getline
allocates a buffer large enough
to hold the line of text and places a pointer to the buffer in line
. You
must free that line later.
So now you have the column and row count. Those two stages (reading the two
sizes) would be better extracted into a separate function.
You now rewind the file and allocate the real array. As stated above, this
should be the first allocation of the program. Following this you read the
data into the array. This might be better split into a separate function.
The read loop can be simpler than yours - you
don't need to get a line and then scan it - just scan it directly:
int ndata = 0;
while (scanf("%lg", &data[ndata++]) == 1) {
// nothing
}
ndata -= columCount * rowCount;
if (ndata != 0) {
printf("%s data values in input\n", ndata > 0 ? "Too few" : "Too many");
return EXIT_FAILURE;
}
And remember to check that the number of doubles read matches what you were
expecting.
Note that scanf
and its associates do not react well to malformed input. There are other ways of reading text but in the simple case (of properly formed data) they are more complicated. Someone else might give you some examples/alternatives.
Finally note that your formatting is inconsistent, as if bits have been cut
and pasted from elsewhere. Copying code from your previous work is not
necessarily bad, but make sure you make the whole thing looks as if it belongs
together.