2
\$\begingroup\$

I am reading through K&R C 2nd Edition, and I am on exercise 1-13. The exercise is to write a program to print a histogram of the lengths of words in its input. I wrote a program that successfully does this; however, after looking at the program, I noticed that it was somewhat messy, and I'm sure that there is a better way to implement it. I'm looking for some tips as to how I can improve programs like this in the future.

Here is the code:

#include <stdio.h>
#define IN 1
#define OUT 0
#define MAXVAL 11

int main(){
    int i, j, c, state;
    int wordLengths[MAXVAL];
    int currentWord = 0;
    int greaterThanMax = 0;

    for(i = 0; i < MAXVAL; i++){
        wordLengths[i] = 0; 
    }

    while((c = getchar()) != EOF){
        ++currentWord;  
        state = IN;
        if(c == '\n' || c == ' ' || c == '\t'){
            state = OUT;    
            --currentWord;
        }   
        if(state == OUT){
            if(currentWord < MAXVAL){
                ++wordLengths[currentWord];
            }
            else{
                ++greaterThanMax;   
            }
            currentWord = 0;
        } 
    }
    for(i = 1; i < MAXVAL; i++){
        printf("%d letter(s): ", i);
        for(j = 0; j <= wordLengths[i] - 1; j++){
            putchar('=');   
        }   
        putchar('\n');
    }
    printf(">%d: ", MAXVAL - 1);
    for(i = 0; i < greaterThanMax; i++){
        putchar('=');   
    }
    return 0;
}
\$\endgroup\$
1
\$\begingroup\$
  • Do not be shy on horizontal spacing. Add space after the keywords (e.g. while (, or if (). Insert space into ){.

    If you place an opening curly bracket on the same line as if, be consistent with else:

      if () {
          ....
      } else {
          ....
      }
    
  • currentWord actually refers to the current word length. Consider renaming.

  • Instead of testing state == OUT, consider adding a word immediately, as soon as state become OUT:

        if (c == '\n' || c == ' ' || c == '\t') {
            if (currentWordLength < MAXVAL) {
                ++wordLengths[currentWordLength - 1]; {
            } else {
                ++greaterThanMax;
            }
            currentWord = 0;
        }
    

    Notice that with this approach you don't need to maintain state explicitly.

  • Every output line, except last one, starts with a single-digit number, but the last one starts with 10, and looks unaligned. Consider printing letter count with "%2d".

  • The greater-than-max line doesn't end with a newline. Some shells (like cmd.exe) automatically add a newline to the last output. Unixish shells do not. On my system an output looks like

    ....
    >10: ==vnp>
    

    Even on Windows, try to redirect your output into a file.

    It is usually a good idea to terminate the output with a newline.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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