Review of elementry program

I have been teaching myself c. This little program parsed out numbers in a file, and prints those divisible by 3. If anyone is willing, can you point out any thing looks like it wrote it in a bad/strange way, errors that I might run into but haven't, or style?

With style, I am not really interested in variable names, formatting, commenting as much as maybe things like choosing a while loop instead of a for loop. I am also interested in any way this might be optimized, not so much through advanced techniques, but more just simple coding choices.

One thing I was wonder was that I put a Macro in my parse_out_num function. Is it bad to use macros in things other than the main function? If its not bad, would I still declare them at the header or in the function?

Thanks to anyone who takes the time to look at this :-) !

Code:

#include <stdio.h> #include <string.h> #include <stdlib.h> void zero_string (char * string, int ary_size) { int idx; for (idx = 0; idx < ary_size; ++idx) { string[idx] = '\0'; } } int parse_out_num(FILE *file, int *array) { char c; int state = 0; #define MAX_NUM_WIDTH 20 int array_idx = 0; char curr_num[MAX_NUM_WIDTH] = ""; int char_index = 0; while (1) { c = fgetc(file); if ( c == EOF ) { break; } // 48-56 is ascii range for digit if ( (c >= 48) && ( c <= 56) ) { state = 1; curr_num[char_index] = c; char_index += 1; } else { // No longer a number if ( state == 1 ) { // printf("%s\n", curr_num); array[array_idx] = atoi(curr_num); array_idx += 1; } state = 0; char_index = 0; zero_string(curr_num, MAX_NUM_WIDTH); } } return(array_idx); } int main() { FILE *numbers; int parsed_num[100]; int index; int numbers_found; numbers = fopen("numbers.txt", "r"); numbers_found = parse_out_num(numbers, parsed_num); for (index = 0; index < numbers_found; ++index) { if ( parsed_num[index] != 0 && (parsed_num[index] % 3) == 0) { printf("%i\n", parsed_num[index]); } } return(0); }

Looks good to me. Except for one thing.

Quote:

Originally Posted by kbrandt

One thing I was wonder was that I put a Macro in my parse_out_num function. Is it bad to use macros in things other than the main function? If its not bad, would I still declare them at the header or in the function?

You can use a macro anywhere you want. But it would be much more normative to have it #defined at the top (ie, globally).

Quote:

Originally Posted by kbrandt

I have been teaching myself c. This little program parsed out numbers in a file, and prints those divisible by 3. If anyone is willing, can you point out any thing looks like it wrote it in a bad/strange way, errors that I might run into but haven't, or style?

With style, I am not really interested in variable names, formatting, commenting as much as maybe things like choosing a while loop instead of a for loop. I am also interested in any way this might be optimized, not so much through advanced techniques, but more just simple coding choices.

One thing I was wonder was that I put a Macro in my parse_out_num function. Is it bad to use macros in things other than the main function? If its not bad, would I still declare them at the header or in the function?

Thanks to anyone who takes the time to look at this :-) !

Your indentation part is not so good so you can rectify it.

Quote:

Originally Posted by BEN10

Your indentation part is not so good so you can rectify it.

Pardon? That indentation is letter perfect. Why don't you try and justify yourself here?

ps. not everybody uses the allman style:

Code:

while (condition) { blah blah }

The OP's alternative is slightly less common but still widespread and acceptable (or preferable, depending on your taste).

Quote:

Originally Posted by MK27

That indentation is letter perfect.

I suppose there is at least one mistake:

Code:

if ( parsed_num[index] != 0 && (parsed_num[index] % 3) == 0) { printf("%i\n", parsed_num[index]); }

Incidentally, this:

Code:

while (1) { c = fgetc(file); if ( c == EOF ) { break; }

is probably better as:

Code:

while ((c = fgetc(file)) != EOF) {

There is no guard against handling a number of consecutive digit characters more than the size of your char array (currently 20). You would overwrite the end of the curr_num array. But, the largest 32-bit signed positive int is only 10 digits anyway (2,147,483,647) which is the largest you'd be able to realistically convert/store into the array so you'd need 11 characters (+1 for the null) but still need to be on guard for potentially more consecutive digit characters by handling that situation in some way.

There is no guard against reading/storing more than 100 numbers in the array.

Code:

void zero_string (char * string, int ary_size) { int idx; for (idx = 0; idx < ary_size; ++idx) { string[idx] = '\0'; } }

and

Code:

zero_string(curr_num, MAX_NUM_WIDTH);

to

Code:

memset(curr_num, 0, sizeof curr_num);

if it would not be better *curr_num = '\0';

Code:

// 48-56 is ascii range for digit if ( (c >= 48) && ( c <= 56) ) {

to

Code:

#include <ctype.h> ... if (isdigit(c)) {

it is undependent from ASCII

if you want read the numbers you may use combination fscanf(file, "%*[^0-9]") and fscanf(file, "%[0-9]", &num) and fscanf(file, "%*[^0-9]") and ...
with this you will exclude atof from the program (and stdlib.h)

Quote:

Originally Posted by MK27

Pardon? That indentation is letter perfect. Why don't you try and justify yourself here?

ps. not everybody uses the allman style:

Code:

while (condition) { blah blah }

The OP's alternative is slightly less common but still widespread and acceptable (or preferable, depending on your taste).

I know that not everybody uses the above style but it's more readable than this.

Code:

while(condition){ blah blah }

Quote:

Originally Posted by BEN10

I know that not everybody uses the above style but it's more readable than this.

True, but there was only one such mistake. It might be better to arrow it out next time everything else is indented properly.