Reverse words

This is a discussion on Reverse words within the C Programming forums, part of the General Programming Boards category; I don't really have a problem as much as I want a critique. If any more experienced programmers, there are ...

  1. #1
    Registered User Dr. Bebop's Avatar
    Join Date
    Sep 2002
    Posts
    96

    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 );
    }
    Processing error: Stupidity detected.
    ------------------------------
    Dr. Bebop
    Windows XP Professional Ed.
    Microsoft Visual Studio 6

  2. #2
    End Of Line Hammer's Avatar
    Join Date
    Apr 2002
    Posts
    6,231
    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?
    When all else fails, read the instructions.
    If you're posting code, use code tags: [code] /* insert code here */ [/code]

  3. #3
    Registered User Dr. Bebop's Avatar
    Join Date
    Sep 2002
    Posts
    96
    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;
    }
    Processing error: Stupidity detected.
    ------------------------------
    Dr. Bebop
    Windows XP Professional Ed.
    Microsoft Visual Studio 6

  4. #4
    End Of Line Hammer's Avatar
    Join Date
    Apr 2002
    Posts
    6,231
    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
    When all else fails, read the instructions.
    If you're posting code, use code tags: [code] /* insert code here */ [/code]

  5. #5
    Registered User Dr. Bebop's Avatar
    Join Date
    Sep 2002
    Posts
    96
    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.
    Processing error: Stupidity detected.
    ------------------------------
    Dr. Bebop
    Windows XP Professional Ed.
    Microsoft Visual Studio 6

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. gethostbyaddr() reverse lookups failing (???)
    By Uncle Rico in forum C Programming
    Replies: 9
    Last Post: 08-19-2005, 09:22 AM
  2. Problem with malloc() and sorting words from text file
    By goron350 in forum C Programming
    Replies: 11
    Last Post: 11-30-2004, 09:01 AM
  3. New Theme
    By XSquared in forum A Brief History of Cprogramming.com
    Replies: 160
    Last Post: 04-01-2004, 07:00 PM
  4. Replies: 7
    Last Post: 03-18-2003, 02:32 PM

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