Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

In Kernighan and Ritchie (the C programming language):

Write a program to copy its input to its output, replacing each string of one or more blanks by a single blank.

I wrote the following code. Do you have advice to improve it?

#include <stdio.h>

/* copy input to output, replacing each 
string of one or more blanks by a single blank */

main(){

    int c;
    double nc;
    char otherBlank;

    otherBlank = 'f';

    while ((c = getchar()) != EOF){
        if (otherBlank == 't' && c != ' '){
            /* read a non blank and had a blank before */
            putchar(' ');
            putchar(c);
            otherBlank = 'f';
        }

        else if (c == ' ')
            /* read a blank */
            otherBlank = 't';

        else
            /* read a non blank and had no blank before */
            putchar(c);

    }


}
share|improve this question
1  
Have you used scanf yet? That would make this really easy. –  Loki Astari May 24 at 17:52
    
I would use a more boolean way to represent otherBlank either stdbool's bool or atleast int with 1/0 perhaps #defined to TRUE/FALSE (old school) –  Karthik T May 25 at 2:31

4 Answers 4

up vote 7 down vote accepted

There a few concerns I have with this program. Let's go through it line by line:

  • You don't declare what you are returning from main(), or what parameters you are going to take in. You should always be declaring these instead of letting the compiler assume them.

    int main(void)
    

    You asked in the comments why we should declare this to return an int and not void. We don't want to just throw away that useful exit status information. There is great answer here that goes into great detail as to why, you should take a look over there.

    "But wait!", you say. "We didn't actually return anything from our main function! How does this black magic work?" Yep, I know. That is just what is declared in the standards.

    C99 & C11 §5.1.2.2(3):

    "...reaching the } that terminates the main() function returns a value of 0."

  • You don't use your nc variable in your program. It should be removed.

  • You declare otherBlank as a char on one line and assign it the character f on the next line. You should be assigning on the same line as you are declaring in this case.

  • Analyzing your program further, it is questionable as to why you aren't using a boolean value to represent otherBlank. You only seem to be using it as a "flag" of a sort, so it makes sense to me to represent it as the bool type. Keep in mind that you will have to include <stdbool.h> if you choose to change this.

  • Further refactoring your program, there is no need for the otherBlank variable. Use a while loop to capture all of the ' ' characters and then output only one ' ' character. Then continue with the printing of the other non-blank characters.

  • You don't check putchar for it's return value, as you should be doing. @Michael went into great detail with this, give his answer a look and an upvote to see why!


Here is what I managed to come up with:

#include <stdio.h>

int main(void)
{
    int c;
    while ((c = getchar()) != EOF) {
        if (c == ' ') {
            while ((c = getchar()) == ' ');
            if (putchar(' ') == EOF) return -1;
            if (c == EOF) break;
        }
        if (putchar(c) == EOF) return -1;
    }
}

Here is how this could be further improved:

  • Use the function fgets() to capture the input once; looping over each character to capture it manually isn't very efficient (and if your program doesn't handle input properly, it could be dangerous).

  • Store the input string in a character array and loop over the characters to test them for ' '. Then make the necessary changes and print out the entire string at once, instead of looping and printing out the individual characters (again, inefficient).

share|improve this answer
    
The return from getchar is an int so that it can hold EOF. So c should be int. Also get_s is not widely supported and is unlikely to reliably capture the "entire input" - use fgets for max compatibility. And be aware that the fgets (or gets_s) will discard each \n. –  William Morris May 24 at 23:40
    
@WilliamMorris EOF is defined in <stdio.h> as #define EOF (-1), and since the range of a char is defined from -128 to 127 it should be safe for the implicit conversion in order to compare c to the int type EOF. And you are probably right about using fgets, I'm just trying to use all this new-fangled C11 standard stuff :P –  syb0rg May 25 at 3:55
    
syb0rg; please can you put in checks for == EOF on the return code of the putchars; see my comment below. –  Michael May 25 at 5:54
    
Using gets_s will fail on some input; see stackoverflow.com/questions/12893774/… for more. –  Michael May 25 at 6:00
1  
@bigTree; there's a nice StackOverflow answer that covers return values stackoverflow.com/questions/204476/… ; note especially the comments about EXIT_SUCCESS and EXIT_FAILURE. –  Michael May 26 at 8:37

Three things

  • you should check the return from putchar and fail with an error message if it fails
  • normally in C you want to use a boolean value not a character for true and false
  • from my understanding of the original question from K&R you seem to have a bug if you have a character then a blank at the end of your file

I see that some other answers have been given with examples that don't include checking the return value. For this reason I'll expand the comment a but more.

If you don't check the return value from putchar it's possible that some of your writes fail then others succeed. This leaves you with a silently corrupted file. This would happen, for example, if your disk filled temporarily during running your program. This is much nastier than just truncating the file to the size of the free space. If the putchar falls the program should visibly fail.

share|improve this answer

I was brewing an answer to this question at the same time as the others, and it overlaps a lot with Syborg's .... but, it is different enough to present it.

The reasons why this is an improvement over your code is that it has just the one variable. Additionally, it has tight loops without conditionals. Even though there is an outer loop, in many instances (long strings of words with few spaces, or words with many huge gaps), the tight loops will make the code highly optimizable. So, here's how I would code it (note that Java is my strongest language, so I normally get help with C....).

int c = getchar();

while (c != EOF){

    do {
        putchar(c);
    } while (c != ' ' && (c = getchar()) != EOF);

    while (c == ' ') {
        c = getchar();
    }
}

I have put this in an ideone to test it...

Using a do-while loop makes things simpler, sometimes.

share|improve this answer
    
The return from getchar is an int so that it can hold EOF. So c should be int. –  William Morris May 24 at 23:54
    
@WilliamMorris - Hmmm ... fixing that now... of course (and I'm sure there's other problems too) –  rolfl May 25 at 0:04

Instead of declaring otherBlank and then assigning to it, just initialize it right away:

char otherBlank = 'f';

This is a bit cleaner and keeps the variable within a lower scope.

Also, nc is declared but unused, so it should be removed. Leaving unused variables around will clutter the code and cause compiler warnings (if warning levels are high enough, which they should be).

share|improve this answer
    
'it keeps the variable in a lower scope' Could you explain me please? –  bigTree May 24 at 14:27
1  
@bigTree: By doing so, it'll be easier to tell what has access to a variable. This is mostly a maintenance thing. For instance, a variable created in a function is in local scope and exists only within that function. While scope is not a large issue for this small program, it's still a useful thing to know about. –  Jamal May 24 at 14:30

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.