Re: Whaddaya think?

DFS <nospam@dfs.com> writes:

On 6/15/2024 6:22 PM, Keith Thompson wrote:

DFS <nospam@dfs.com> writes:

I want to read numbers in from a file, say:
>
47 185 99 74 202 118 78 203 264 207 19 17 34 167 148 54 297 271 118
245 294 188 140 134 251 188 236 160 48 189 228 94 74 27 168 275 144
245 178 108 152 197 125 185 63 272 239 60 242 56 4 235 244 144 69 195
32 4 54 79 193 282 173 267 8 40 241 152 285 119 259 136 15 83 21 78 55
259 137 297 15 141 232 259 285 300 153 16 4 207 95 197 188 267 164 195
7 104 47 291
>
>
This code:
1 opens the file
2 fscanf thru the file to count the number of data points
3 allocate memory
4 rewind and fscanf again to add the data to the int array
>
>
Any issues with this method?
>
Any 'better' way?
>
Thanks

In a quick test, your code compiles without errors and runs
correctly
with your input.  I do get a warning about argc being unused, which you
should address.

>
-Wall doesn't warn about that, but -Wall -Wextra does.

True (at least for gcc).

In the bigger program of which this is a part, argc IS used.

I suggest that argc *should* have been used in this program.

----------------------------------------------------------
#include <stdio.h>
#include <stdlib.h>
>
int main(int argc, char *argv[]) {
>
int N=0, i=0, j=0;

The usual convention is to use all-caps for macro names.  Calling
your
variable N is not a real problem, but could be slightly confusing.
N is the number of integers in the input.  i is an index.  j is a
value
read from the file.  That's not at all clear from the names.
I suggest using longer and more descriptive names in lower case.
"N" could be "count".  "i" is fine for an index, but "j" could be
"value".

>
>
N is used in statistics, and this is a stats program.

OK, using the names i and j suggests they're used similarly when they're
not.

[...]

I looked for an easy way to lock it while reading, but as I understand
flock() it only places an 'advisory lock' on the file, and other
processes are still free to modify it.

Locking the file probably isn't worth the effort.  If I were doing this
kind of thing, I'd just keep in the back of my mind that if some other
process modifies the file while my program is running, bad things can
happen.  For something that's going to be used in production, more care
is appropriate.

[...]

A minor style point: a return statement doesn't require parentheses.
IMHO using parentheses make it look too much like a function call.  I'd
write `return 0;`, or more likely I'd just omit it, since falling off
the end of main does an implicit `return 0;` (starting in C99).

>
Can't omit it.  It's required by my brain.

OK, that's harmless -- but be prepared to see the "return 0;" omitted in
code written by other people.

>

}

A method that doesn't require rescanning the input file is to
initially
allocate some reasonable amount of memory, then use realloc() to
expand the array as needed.  Doubling the array size is probably
reasonable.  It will consume more memory than a single allocation.

>
Done in a way, as you'll see below.
>
>
Thanks for the thorough analysis and good tips.
>
>
Updated
* dropped 2 variable declarations
* allocate 'on the fly'
* one fscanf thru the file
* 4 less lines of code (not incl brackets)
>
----------------------------------------------------------
#include <stdio.h>
#include <stdlib.h>
>
int main(int argc, char *argv[]) {
>
    int N=0;
    int *nums = malloc(2 * sizeof(int));

Again, I really recommend

    int *nums = malloc(2 * sizeof *nums);

    FILE* datafile = fopen(argv[1], "r");

Undefined behavior if the program is invoked with no arguments.
No check whether fopen succeeded.

    while(fscanf(datafile, "%d", &nums[N++]) == 1){

I've already discussed the problems of using fscanf for numeric input.

        nums = realloc(nums, (N+1) * sizeof(int));

I'd think about moving the increment of N so that I don't have to use
"N+1" in realloc or add "N--;" at the end.  I haven't taken the time to
work out the details, but I think this can be done more cleanly.

Calling realloc() every time could be wasteful.  (On my system, realloc
reallocates at about 25 bytes and not again until about 128 kbytes, but
don't rely on that.)  A common scheme is to call realloc() to double the
buffer size as needed.  This requires keep track of the size of the
buffer and the number of elements used separately.  But for a
quick-and-dirty demo, calling realloc() every time isn't too bad.

    }
    fclose (datafile);
>
    N--;
    for(int i=0;i<N;i++) {
        printf("%d.%d  ", i+1, nums[i]);
    }
    free(nums);
>
    printf("\n");
    return 0;
>
}
----------------------------------------------------------

You changed the output format.  Your original program printed each
number on line line (with extra blank lines at the top and bottom for
some reason); this program prints all the numbers on a single line.
Neither is right or wrong, but consistency is nice -- and very long
lines can cause problems in looking at the output.

--
Keith Thompson (The_Other_Keith) Keith.S.Thompson+u@gmail.com
void Void(void) { Void(); } /* The recursive call of the void */