473,320 Members | 2,048 Online
Bytes | Software Development & Data Engineering Community
Post Job

Home Posts Topics Members FAQ

Join Bytes to post your question to a community of 473,320 software developers and data experts.

code review/comments please

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[i] );
}
fprintf(stdout, "\n");

/*
** clean up memory
*/
for( i=0; i<word_count; i++ )
{
free( sentence[i] );
sentence[i] = 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[i] );
sentence[i] = NULL;
}
return 1;
}
memcpy( sentence[(*word_count)-1], str_p,
strlen(str_p)+1 );
}
return 0;
}
=============================================

Nov 13 '05 #1
1 2090

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[i] );
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[i] );
sentence[i] = 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[i] );
sentence[i] = 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

Nov 13 '05 #2

This thread has been closed and replies have been disabled. Please start a new discussion.

Similar topics

2
by: Dave Patton | last post by:
I'd appreciate any feedback on http://www.elac.bc.ca/ particularly in regards to how the pages are marked up. The markup is valid HTML 4.01 strict, but that doesn't mean I've done things using...
11
by: Guotao Luan | last post by:
Hello All: I notice that there have been frequent questions being asked about template here so I guess it is okay to post this message here. I just wrote a c++ tempalte tutorial/review, I would...
18
by: Ben Hanson | last post by:
I have created an open source Notepad program for Windows in C++ that allows search and replace using regular expressions (and a few other extras). It is located at...
6
by: Michael Winter | last post by:
Out of curiosity, did anyone review the second attempt I made at the DOM 2 Events wrapper, posted at the end of last month? I posted it in the previous thread rather than a new one and I have the...
2
by: rked | last post by:
I get nameSPAN1 is undefined when I place cursor in comments box.. <%@ LANGUAGE="VBScript" %> <% DIM ipAddress ipAddress=Request.Servervariables("REMOTE_HOST") %> <html> <head> <meta...
192
by: Vortex Soft | last post by:
http://www.junglecreatures.com/ Try it and tell me what's happenning in the Microsoft Corporation. Notes: VB, C# are CLS compliant
3
by: Larry Serflaten | last post by:
I am planning to share a Cards.DLL out on Planet Source Code and GotDotNet. But before I send it out to the public I would like to get a peer review to be sure it works as intended, and to avoid...
1
by: JM | last post by:
I'm studying for webdeveloper. The trainer only comments when it doesn't work, but I am also interested in comments on programs or pieces of code that work : can it work better, faster, shorter,...
29
by: Virtual_X | last post by:
As in IEEE754 double consist of sign bit 11 bits for exponent 52 bits for fraction i write this code to print double parts as it explained in ieee754 i want to know if the code contain any...
0
by: DolphinDB | last post by:
Tired of spending countless mintues downsampling your data? Look no further! In this article, you’ll learn how to efficiently downsample 6.48 billion high-frequency records to 61 million...
0
by: ryjfgjl | last post by:
ExcelToDatabase: batch import excel into database automatically...
0
isladogs
by: isladogs | last post by:
The next Access Europe meeting will be on Wednesday 6 Mar 2024 starting at 18:00 UK time (6PM UTC) and finishing at about 19:15 (7.15PM). In this month's session, we are pleased to welcome back...
0
by: Vimpel783 | last post by:
Hello! Guys, I found this code on the Internet, but I need to modify it a little. It works well, the problem is this: Data is sent from only one cell, in this case B5, but it is necessary that data...
0
by: jfyes | last post by:
As a hardware engineer, after seeing that CEIWEI recently released a new tool for Modbus RTU Over TCP/UDP filtering and monitoring, I actively went to its official website to take a look. It turned...
0
by: ArrayDB | last post by:
The error message I've encountered is; ERROR:root:Error generating model response: exception: access violation writing 0x0000000000005140, which seems to be indicative of an access violation...
0
by: Shællîpôpï 09 | last post by:
If u are using a keypad phone, how do u turn on JavaScript, to access features like WhatsApp, Facebook, Instagram....
0
by: af34tf | last post by:
Hi Guys, I have a domain whose name is BytesLimited.com, and I want to sell it. Does anyone know about platforms that allow me to list my domain in auction for free. Thank you
0
by: Faith0G | last post by:
I am starting a new it consulting business and it's been a while since I setup a new website. Is wordpress still the best web based software for hosting a 5 page website? The webpages will be...

By using Bytes.com and it's services, you agree to our Privacy Policy and Terms of Use.

To disable or enable advertisements and analytics tracking please visit the manage ads & tracking page.