Thread: Reverse words

I don't really have a problem as much as I want a critique. If any more experienced programmers, there are probably a lot, could look at my code and see if they can find any inconsistencies or potential problems that would be great. I've looked over it several times and walked through it with my debugger, so far it looks good.

What this program does is read a string of words from either the command line or standard input and reverses the words in that string, then prints the reversed string to either standard output or a file that the user inputs. So if you put

$prog "Hi, my name is Bebop" out.txt

the program will write to out.txt

Bebop is name my Hi,

Sorry about it not having many comments, I don't add them until the program is actually working and debugged and I'm satisfied that it's all good.

Code:

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

void ERROR( char *msg, char *mem );
void word_rev( char *line, FILE *str );

int main( int argc, char *argv[] )
{
	FILE *out;
	char *str_buf, *nl;
	int alloc = 0;

	/* $prog "string to rev" file */
	if( argc == 3 ) {
		out = fopen( argv[2], "w" );
		if( out == NULL )
			ERROR( "Error opening output file\n", NULL );

		str_buf = argv[1];
	}
	/* $prog "string to rev" */
	else if( argc == 2 ) {
		out = stdout;
		str_buf = argv[1];
	}
	/* $prog */
	else if( argc == 1 ) {
		out = stdout;

		str_buf = malloc( 1024 );
		if( str_buf == NULL )
			ERROR( "Error allocating memory\n", NULL );

		fprintf( stdout, "Enter a string: " );
		if( fgets( str_buf, 1024, stdin ) == NULL )
			ERROR( "Error reading from keyboard\n", str_buf );

		if( (nl = strchr( str_buf, '\n' )) != NULL )
			*nl = '\0';

		alloc = 1;
	}
	/* Bad input */
	else
		ERROR( "usage: <string> <output file>\n", NULL );

	word_rev( str_buf, out );

	if( alloc == 1 )
		free( str_buf );

	return EXIT_SUCCESS;
}

void word_rev( char *line, FILE *str )
{
	int n_words = 0;
	char *stack[1024], *sep;

	sep = strtok( line, " " );
	/* Save the words */
	while( sep != NULL && n_words < 1024 ) {
		stack[n_words++] = sep;
		sep = strtok( NULL, " " );
	}

	/* Play them back in reverse */
	while( n_words > 0 ) {
		fprintf( str, "%s", stack[--n_words] );

		if( n_words == 0 )
			fputc( '\n', str );
		else
			fputc( ' ', str );
	}
}

void ERROR( char *msg, char *mem )
{
	fprintf( stderr, msg );
	if( mem != NULL )
		free( mem );
	exit( EXIT_FAILURE );
}

Here's a few small points (nit picking really ). Some (maybe most) are simply comments showing how you might have done something in a different way, rather than showing problems.

Here goes...

Not that this point is really relevant to your program, but to show you what else you can do (if you already know, just ignore)...
With your ERROR function, you could extend it to have a dynamic number of args, allowing you to pass a formatted string, like printf() uses. You obviously don't need this in your current program, but it might come in handy else where.

Code:

#include <stdio.h>
#include <stdarg.h>

void ERROR(char *mem, char *format, ...)
{
    va_list args;
    
    if( mem != NULL )
        free( mem );

    va_start(args, format);
    
    fprintf(stderr, "ERROR : ");
    vfprintf(stderr, format, args);

    va_end(args);
    exit( EXIT_FAILURE );
}

Initialise str_buf as NULL, then use it in the ERROR function call, saving you coding NULL as the second parm in the appropriate places. this also negates the need for the alloc variable.
>>char *str_buf = NULL;
>>ERROR( "Error allocating memory\n", str_buf );
instead of
>>ERROR( "Error allocating memory\n", NULL );

This piece of code:
>>if( (nl = strchr( str_buf, '\n' )) != NULL )
>> *nl = '\0';
can also be written like this:
>>str_buf[strcspn(str_buf,"\n")] = '\0';

Define something that means true/false, rather than hard coding 1 and 0 for the "alloc" variable.
Along the same lines, remove the hard coded reference to 1024.

Code:

while( n_words > 0 ) 
{
    fprintf( str, "%s", stack[--n_words] );
    
    if( n_words == 0 )
        fputc( '\n', str );
    else
        fputc( ' ', str );
}

In this piece of code, there's no point in testing for n_words==0 within the loop, it's a waste.
Just add the space character to the fprintf() call, and output the newline character after the loop has finished.

Now, for fun, how about writing a version that doesn't alter the original string?

Thanks for the suggestions, I used them all except for the variable argument error function, that seems like a little much for this program. You can probably figure out what the changes look like, so I'll just post the main function modified to not alter the original string. I did it by making a copy of the string and passing the copy to word_rev. I do have a question about passing str_buf instead of NULL to the error function, doesn't that kind of hide what it's really doing? I thought that using NULL made it easier to understand that no memory was going to be freed.

Code:

int main( int argc, char *argv[] )
{
	FILE *out;
	char *str_buf = NULL, *arg = NULL;

	/* $prog "string to rev" file */
	if( argc == 3 ) {
		out = fopen( argv[2], "w" );
		if( out == NULL )
			ERROR( "Error opening output file\n", str_buf );

		str_buf = argv[1];
	}
	/* $prog "string to rev" */
	else if( argc == 2 ) {
		out = stdout;
		str_buf = argv[1];
	}
	/* $prog */
	else if( argc == 1 ) {
		out = stdout;

		arg = malloc( ALLOC_SIZE );
		if( arg == NULL )
			ERROR( "Error allocating memory\n", arg );

		str_buf = malloc( ALLOC_SIZE );
		if( str_buf == NULL )
			ERROR( "Error allocating memory\n", str_buf );

		fprintf( stdout, "Enter a string: " );
		if( fgets( str_buf, ALLOC_SIZE, stdin ) == NULL )
			ERROR( "Error reading from keyboard\n", str_buf );

		str_buf[strcspn( str_buf, "\n" )] = '\0';
		strcpy( arg, str_buf );
	}
	/* Bad input */
	else
		ERROR( "usage: <string> <output file>\n", str_buf );

	word_rev( arg, out );

	free( str_buf );
	free( arg );

	return EXIT_SUCCESS;
}

I do have a question about passing str_buf instead of NULL to the error function, doesn't that kind of hide what it's really doing? I thought that using NULL made it easier to understand that no memory was going to be freed.

It gives the same result, it's just a style thing, and I suppose as such, it's your choice. Having said that, if another programmer was to amend your code in the future, they might inadvertantly pass str_buf to ERROR() without realising it wasn't set to a defined value. Stupid I know, but you can idiot proof your code by simplifying the functions calls, making them all look the same. That way, a cut/paste of a line containing ERROR() will not result in program death.

With regards to your string duping, I'd suggest you do the malloc() + free() for the duplicate copy in the reverse function. There is no need for main to know anything about a second copy of the string.

I don't think I was clear enough when I mentioned about not altering the string. How about writing one that doesn't alter the string at all (therefore no need for a dup copy). This will mean you can't use strtok().

Have fun

I've done that already, but it still alters the original string in place by reversing all of the characters and then reversing the characters in each word. I thought you meant don't change the original at all when you said don't alter the string.