code review/comments please

Discussion in 'C Programming' started by djinni, Sep 27, 2003.

  1. djinni

    djinni Guest

    It's been a while since I've written anything in C, and
    I was hoping to get some feedback on the following code.
    As far as I can tell, the program works fine, but I'm not
    sure I'm cleaning all the memory up properly, and I think
    the "tokenize_my_string" function can probably be made better.

    This program prompts the user for a text sentence,
    then re-orders the words in a random fashion...

    ======================================================
    #include <stdlib.h>
    #include <stdio.h>
    #include <string.h>
    #include <time.h>

    #define DEBUG 0

    #define MAX_NOF_TOK 256 /* max number of words */
    #define MAX_TOK_LEN 256 /* max word length */
    #define MAX_MSG_LEN (MAX_NOF_TOK * MAX_TOK_LEN)
    #define RAND_FACTOR 5

    void sort_my_string( char** sentence, int word_count );
    int tokenize_my_string( char* buffer,
    char** sentence,
    int* word_count );

    int main(void)
    {
    char buffer[ MAX_MSG_LEN ];
    char *sentence[ MAX_NOF_TOK ];
    char *first_string;
    int i=0, word_count=0;

    /*
    ** output instructions
    */
    fprintf( stdout, "Type in the text:\n\t" );
    fflush( stdout );

    /*
    ** get input string from user
    */
    if( fgets( buffer, MAX_MSG_LEN, stdin ) == NULL )
    {
    fprintf(stderr, "learn how to type jackass!\n" );
    return EXIT_FAILURE;
    }

    /*
    ** store buffer so strtok doesn't walk all over it
    */
    first_string = malloc(strlen(buffer)+1);
    if( first_string == NULL )
    {
    fprintf(stderr, "you need more memory dude\n");
    return EXIT_FAILURE;
    }
    memcpy( first_string, buffer, strlen(buffer)+1 );

    /*
    ** pesky newline...
    */
    if( buffer[strlen(buffer)-1] == '\n' )
    {
    buffer[strlen(buffer)-1] = '\0';
    }

    /*
    ** break up the string into words
    ** and store each word as an element
    ** in the "sentence" array
    **
    ** remember that this allocates memory for "sentence",
    ** so don't forget to free() it!!!
    */
    if( tokenize_my_string( buffer, sentence, &word_count ) )
    {
    fprintf(stderr, "tokenize_my_string failed!\n" );
    free( first_string );
    first_string = NULL;
    return EXIT_FAILURE;
    }

    sort_my_string( sentence, word_count );

    fprintf(stdout,"randomized string looks like:\n\t");
    /*
    ** print the sorted string
    */
    for( i=0; i<word_count; i++ )
    {
    fprintf(stdout, "%s ", sentence );
    }
    fprintf(stdout, "\n");

    /*
    ** clean up memory
    */
    for( i=0; i<word_count; i++ )
    {
    free( sentence );
    sentence = NULL;
    }
    free( first_string );
    first_string = NULL;

    return EXIT_SUCCESS;
    }


    void sort_my_string( char** s, int word_count )
    {
    char *tmp;
    int ind_from, ind_to;
    int rand_ind = (word_count * RAND_FACTOR);
    srand( time(0) );

    while( --rand_ind > 0 )
    {
    ind_from = rand() % word_count;
    ind_to = rand() % word_count;
    if( ind_from == ind_to )
    continue;
    tmp = s[ind_to];
    s[ind_to] = s[ind_from];
    s[ind_from] = tmp;
    }

    return;
    }

    int tokenize_my_string( char* buffer,
    char** sentence,
    int *word_count )
    {
    char *str_p;
    int i;

    /*
    ** get the first token...
    */
    if( (str_p = strtok( buffer, " " )) == NULL )
    {
    fprintf( stderr, "it broke here: %d\n", __LINE__ );
    return 1;
    }
    (*word_count) = 1;

    /*
    ** get space for the first token in our sorted string
    */
    sentence[0] = malloc( strlen(str_p)+1 );
    if( sentence[0] == NULL )
    {
    fprintf(stderr, "malloc failed: %d\n", __LINE__ );
    return 1;
    }
    memcpy( sentence[0], str_p, strlen(str_p)+1 );

    /*
    ** get the rest of the tokens
    */
    while( ((str_p = strtok(NULL, " "))!=NULL) &&
    (++(*word_count) < MAX_NOF_TOK) )
    {
    /*
    ** make some room
    */
    sentence[(*word_count)-1] = malloc( strlen(str_p)+1 );
    if( sentence[(*word_count)-1] == NULL )
    {
    fprintf(stderr, "uh oh, not again! %d\n", __LINE__ );
    for( i=0; i< (*word_count)-1; i++ )
    {
    free( sentence );
    sentence = NULL;
    }
    return 1;
    }
    memcpy( sentence[(*word_count)-1], str_p,
    strlen(str_p)+1 );
    }
    return 0;
    }
    =============================================
     
    djinni, Sep 27, 2003
    #1
    1. Advertising

  2. On Sat, 27 Sep 2003, djinni wrote:
    >
    > It's been a while since I've written anything in C, and
    > I was hoping to get some feedback on the following code.


    Looks pretty good. Some comments.

    > This program prompts the user for a text sentence,
    > then re-orders the words in a random fashion...
    >
    > ======================================================
    > #include <stdlib.h>
    > #include <stdio.h>
    > #include <string.h>
    > #include <time.h>
    >
    > #define DEBUG 0
    >
    > #define MAX_NOF_TOK 256 /* max number of words */
    > #define MAX_TOK_LEN 256 /* max word length */
    > #define MAX_MSG_LEN (MAX_NOF_TOK * MAX_TOK_LEN)
    > #define RAND_FACTOR 5
    >
    > void sort_my_string( char** sentence, int word_count );
    > int tokenize_my_string( char* buffer,
    > char** sentence,
    > int* word_count );
    >
    > int main(void)
    > {
    > char buffer[ MAX_MSG_LEN ];


    Putting a 65536-character array as an automatic variable
    is usually not wise, IMHO. I suggest making this array
    into a dynamically-allocated buffer.

    > char *sentence[ MAX_NOF_TOK ];
    > char *first_string;
    > int i=0, word_count=0;
    >
    > /*
    > ** output instructions
    > */
    > fprintf( stdout, "Type in the text:\n\t" );
    > fflush( stdout );
    >
    > /*
    > ** get input string from user
    > */
    > if( fgets( buffer, MAX_MSG_LEN, stdin ) == NULL )
    > {
    > fprintf(stderr, "learn how to type jackass!\n" );
    > return EXIT_FAILURE;
    > }
    >
    > /*
    > ** store buffer so strtok doesn't walk all over it
    > */
    > first_string = malloc(strlen(buffer)+1);
    > if( first_string == NULL )
    > {
    > fprintf(stderr, "you need more memory dude\n");
    > return EXIT_FAILURE;
    > }
    > memcpy( first_string, buffer, strlen(buffer)+1 );


    Silly and inefficient way of writing

    strcpy(first_string, buffer);

    > /*
    > ** pesky newline...
    > */
    > if( buffer[strlen(buffer)-1] == '\n' )
    > {
    > buffer[strlen(buffer)-1] = '\0';
    > }


    Inefficient re-calculation of 'strlen'. Try

    char *tmp; /* at start of block */
    if ((tmp = strchr(buffer, '\n')) != NULL)
    *tmp = '\0';

    > /*
    > ** break up the string into words
    > ** and store each word as an element
    > ** in the "sentence" array
    > **
    > ** remember that this allocates memory for "sentence",
    > ** so don't forget to free() it!!!
    > */
    > if( tokenize_my_string( buffer, sentence, &word_count ) )
    > {
    > fprintf(stderr, "tokenize_my_string failed!\n" );
    > free( first_string );
    > first_string = NULL;


    IMHO this re-initialization of first_string is silly,
    both in general and *especially* in this case, since you're
    immediately discarding not only the value of 'first_string',
    but the value of every other object in the program -- by
    returning from 'main'!

    > return EXIT_FAILURE;
    > }
    >
    > sort_my_string( sentence, word_count );
    >
    > fprintf(stdout,"randomized string looks like:\n\t");


    A.K.A. printf

    > /*
    > ** print the sorted string
    > */
    > for( i=0; i<word_count; i++ )
    > {
    > fprintf(stdout, "%s ", sentence );


    A.K.A. printf

    > }
    > fprintf(stdout, "\n");


    If you want to be fancy, consider what happens when the
    user enters more than one screen's-width of text. Should
    you bother to insert line breaks after each, say, 70
    characters (at word breaks, naturally)? Or is appearance
    not necessarily a goal for this program?

    > /*
    > ** clean up memory
    > */
    > for( i=0; i<word_count; i++ )
    > {
    > free( sentence );
    > sentence = NULL;
    > }
    > free( first_string );
    > first_string = NULL;


    Again with the useless NULLing right before program
    termination. I don't like it -- it just increases
    the code size without having any beneficial effects.

    > return EXIT_SUCCESS;
    > }
    >
    >
    > void sort_my_string( char** s, int word_count )


    As a general rule, I would try to put the function
    definitions in such a small project in order as
    they're called: i.e., put the parsing function
    before the sorting function. It's slightly easier
    on the eyes, IMO. (A larger project might sort
    its functions by category, or alphabetically; again
    so that individual functions are easy to find.)

    > {
    > char *tmp;
    > int ind_from, ind_to;
    > int rand_ind = (word_count * RAND_FACTOR);
    > srand( time(0) );


    It makes more sense to put the above initialization
    of the RNG into main(), rather than into a utility
    function.

    > while( --rand_ind > 0 )
    > {
    > ind_from = rand() % word_count;
    > ind_to = rand() % word_count;
    > if( ind_from == ind_to )
    > continue;
    > tmp = s[ind_to];
    > s[ind_to] = s[ind_from];
    > s[ind_from] = tmp;
    > }


    Not a correct way of getting a random
    distribution, of course; but that doesn't
    look all that important.

    >
    > return;
    > }
    >
    > int tokenize_my_string( char* buffer,
    > char** sentence,
    > int *word_count )


    If you wanted to make this function more robust,
    and simultaneously make your program more easily
    maintainable, you'd add a parameter to this
    prototype giving the maximum size of the 'sentence'
    array. Right now you're having to hard-code
    MAX_NOF_TOK into the function, which is a bad thing.

    > {
    > char *str_p;
    > int i;
    >
    > /*
    > ** get the first token...
    > */
    > if( (str_p = strtok( buffer, " " )) == NULL )
    > {
    > fprintf( stderr, "it broke here: %d\n", __LINE__ );
    > return 1;


    This should never, ever happen, of course. Unless
    'buffer' is the empty string, or NULL. So it might
    make more sense to check for those conditions, instead
    of using 'strtok'. In general, 'strtok' is evil, and
    you should try not to use it -- because it modifies
    your input data, if you ever wanted to change the program
    to use that data for something else after parsing (such
    as including it in an error message or debugging dump),
    you'd have to change the parts that use 'strtok' also.

    > }
    > (*word_count) = 1;
    >
    > /*
    > ** get space for the first token in our sorted string
    > */
    > sentence[0] = malloc( strlen(str_p)+1 );
    > if( sentence[0] == NULL )
    > {
    > fprintf(stderr, "malloc failed: %d\n", __LINE__ );
    > return 1;
    > }
    > memcpy( sentence[0], str_p, strlen(str_p)+1 );


    Again with the 'strcpy'.

    > /*
    > ** get the rest of the tokens
    > */
    > while( ((str_p = strtok(NULL, " "))!=NULL) &&
    > (++(*word_count) < MAX_NOF_TOK) )
    > {
    > /*
    > ** make some room
    > */
    > sentence[(*word_count)-1] = malloc( strlen(str_p)+1 );
    > if( sentence[(*word_count)-1] == NULL )
    > {
    > fprintf(stderr, "uh oh, not again! %d\n", __LINE__ );
    > for( i=0; i< (*word_count)-1; i++ )
    > {
    > free( sentence );
    > sentence = NULL;
    > }
    > return 1;


    Since this error return signifies "out of memory", and
    the first error return signifies "invalid input", I would
    give them different return codes (i.e., not both 'return 1;').
    And it's typical to make error codes negative, IME. YMMV.

    > }
    > memcpy( sentence[(*word_count)-1], str_p,
    > strlen(str_p)+1 );


    Again, 'strcpy'.

    > }
    > return 0;
    > }


    That's the only bad parts I see at the moment.
    Weird 'strcpy' replacement, hard-coded constant,
    misplaced 'srand', evil 'strtok' usage, weird
    'printf' replacement, and fixed buffer sizes.

    Oh, and 4-space tabs is my preferred style.
    Your code is pretty good w.r.t. line length,
    so adding the extra couple indentation levels
    wouldn't adversely affect readability.

    Hope this helps,
    -Arthur
     
    Arthur J. O'Dwyer, Sep 27, 2003
    #2
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Guotao Luan
    Replies:
    11
    Views:
    671
    Markus Dehmann
    Jun 16, 2004
  2. Kevin Wan
    Replies:
    5
    Views:
    729
    Kevin Wan
    Jan 17, 2007
  3. g
    Replies:
    3
    Views:
    684
  4. www
    Replies:
    51
    Views:
    1,488
  5. Mclaren Fan
    Replies:
    2
    Views:
    653
    Richard Cornford
    Nov 8, 2011
Loading...

Share This Page