Review of elementry program

This is a discussion on Review of elementry program within the C Programming forums, part of the General Programming Boards category; I have been teaching myself c. This little program parsed out numbers in a file, and prints those divisible by ...

  1. #1
    Registered User
    Join Date
    May 2009
    Location
    Boston
    Posts
    12

    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);
    }

  2. #2
    spurious conceit MK27's Avatar
    Join Date
    Jul 2008
    Location
    segmentation fault
    Posts
    8,300
    Looks good to me. Except for one thing.

    Quote Originally Posted by kbrandt View Post
    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).
    C programming resources:
    GNU C Function and Macro Index -- glibc reference manual
    The C Book -- nice online learner guide
    Current ISO draft standard
    CCAN -- new CPAN like open source library repository
    3 (different) GNU debugger tutorials: #1 -- #2 -- #3
    cpwiki -- our wiki on sourceforge

  3. #3
    DESTINY BEN10's Avatar
    Join Date
    Jul 2008
    Location
    in front of my computer
    Posts
    804
    Quote Originally Posted by kbrandt View Post
    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.
    HOPE YOU UNDERSTAND.......

    By associating with wise people you will become wise yourself
    It's fine to celebrate success but it is more important to heed the lessons of failure
    We've got to put a lot of money into changing behavior


    PC specifications- 512MB RAM, Windows XP sp3, 2.79 GHz pentium D.
    IDE- Microsoft Visual Studio 2008 Express Edition

  4. #4
    spurious conceit MK27's Avatar
    Join Date
    Jul 2008
    Location
    segmentation fault
    Posts
    8,300
    Quote Originally Posted by BEN10 View Post
    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).
    C programming resources:
    GNU C Function and Macro Index -- glibc reference manual
    The C Book -- nice online learner guide
    Current ISO draft standard
    CCAN -- new CPAN like open source library repository
    3 (different) GNU debugger tutorials: #1 -- #2 -- #3
    cpwiki -- our wiki on sourceforge

  5. #5
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    21,327
    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) {
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  6. #6
    Registered User hk_mp5kpdw's Avatar
    Join Date
    Jan 2002
    Location
    Northern Virginia/Washington DC Metropolitan Area
    Posts
    3,794
    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.
    "Owners of dogs will have noticed that, if you provide them with food and water and shelter and affection, they will think you are god. Whereas owners of cats are compelled to realize that, if you provide them with food and water and shelter and affection, they draw the conclusion that they are gods."
    -Christopher Hitchens

  7. #7
    Registered User
    Join Date
    Apr 2009
    Location
    Russia
    Posts
    116
    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)
    Last edited by c.user; 05-22-2009 at 08:39 PM.

  8. #8
    DESTINY BEN10's Avatar
    Join Date
    Jul 2008
    Location
    in front of my computer
    Posts
    804
    Quote Originally Posted by MK27 View Post
    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
    }
    HOPE YOU UNDERSTAND.......

    By associating with wise people you will become wise yourself
    It's fine to celebrate success but it is more important to heed the lessons of failure
    We've got to put a lot of money into changing behavior


    PC specifications- 512MB RAM, Windows XP sp3, 2.79 GHz pentium D.
    IDE- Microsoft Visual Studio 2008 Express Edition

  9. #9
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    21,327
    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.
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Using variables in system()
    By Afro in forum C Programming
    Replies: 8
    Last Post: 07-03-2007, 12:27 PM
  2. BOOKKEEPING PROGRAM, need help!
    By yabud in forum C Programming
    Replies: 3
    Last Post: 11-16-2006, 10:17 PM
  3. Can someome help me with a program please?
    By WinterInChicago in forum C++ Programming
    Replies: 3
    Last Post: 09-21-2006, 10:58 PM
  4. review my telephone network simulation program
    By debuger2004 in forum C Programming
    Replies: 3
    Last Post: 06-20-2003, 01:26 PM
  5. My program, anyhelp
    By @licomb in forum C Programming
    Replies: 14
    Last Post: 08-14-2001, 10:04 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21