code review/comments please

[djinni]

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

 

[Arthur J. O'Dwyer]

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