2
\$\begingroup\$

I thought I'd give it a shot. What I came up with is from a culmination of my last two submissions, incorporating most of the feedback from this community, including better naming of variables, initialization, and making my program a bit more user-friendly. I'm not sure which optimizations I can make or if there are any issues with my code.

Any feedback is welcome!

#include <stdio.h>

/* print the length of words as input to a histogram with vertical bars */
int main(void) {
  int c, i, j;
  int word_length = 0;
  int max_word_frequency = 0;
  int histogram[10] = {0};

  putchar('>');
  while ((c = getchar()) != EOF && word_length < 10) {
    if (c != ' ' && c != '\n' && c != '\t') {
      ++word_length;
    }
    else {
      ++histogram[word_length];
      word_length = 0;
    }
  }
  for (i = 0; i < 10; ++i) {
    printf("%d ", i);
    if (histogram[i] > max_word_frequency)
      max_word_frequency = histogram[i];
  }
  putchar('\n');
  for (i = 0; i < max_word_frequency; ++i) {
    for (j = 0; j < 9; ++j) {
      if (histogram[j] > 0) {
         printf("x ");
         histogram[j] = histogram[j] -1;
      }
      else {
         printf("  ");
      }
    }
  putchar('\n');
  }
}
\$\endgroup\$
4
\$\begingroup\$

main()

Your main() does three things: print a prompt, count the frequencies, and print the histogram. The latter two, in particular, are well-defined complex tasks, they each deserve to be a function.

If the input is redirected from a file, rather than typed directly on the console, then the > prompt just messes up the output. To see if standard input is a console, you should test isatty() on Unix, or _isatty() on Windows.

int main() {
    int histogram[21];
    if (isatty(fileno(stdin))) {
        putchar('>');
    }
    count_word_lengths(stdin, histogram, sizeof histogram / sizeof histogram[0]);
    print_histogram(histogram, sizeof histogram / sizeof histogram[0]);
}

Counting

If the input ends abruptly with a word followed by EOF, with no intervening whitespace, then the last word doesn't get counted.

Instead of if (c != ' ' && c != '\n' && c != '\t'), consider using tests like isspace() and ispunct(). By the way, you are treating punctuation as if they were part of words, which is probably not the right behaviour.

If the input contains any word with length 10 or greater, then the counting terminates prematurely with no indication that anything went wrong. (Personally, I'd choose to count all excessively long words as if it were the maximum length supported by the histogram array.)

Printing

Destroying the histogram data as you print is a bad idea, especially if it will cause a print_histogram() function to wipe out its input parameter.

Once you fix that, the printf("x ") and printf(" ") would be better with a ternary expression.

for (int row = 1; row <= max_word_frequency; ++row) {
    for (int i = 0; i < 9; ++i) {
        printf(row <= histogram[i] ? "x " : "  ");
    }
    putchar('\n');
}
\$\endgroup\$
  • \$\begingroup\$ Good catch on the "input ends abruptly with a word followed by EOF" and what to do about "word with length 10 or greater". \$\endgroup\$ – chux Sep 9 '15 at 22:05
2
\$\begingroup\$

Rather the use 10 throughout the code, define a constant in one place.
Certainly makes code easier to update to a new size and self-documents the "meaning" of 10

#define HISTOGRAM_WIDTH 10
int histogram[HISTOGRAM_WIDTH] = {0};
...
while ((c = getchar()) != EOF && word_length < HISTOGRAM_WIDTH) {
  ...
  }
for (i = 0; i < HISTOGRAM_WIDTH; ++i) {
  ...
for (j = 0; j < HISTOGRAM_WIDTH - 1; ++j) {
  ...

BTW: This makes the j < HISTOGRAM_WIDTH - 1 suspicious. Should code have been j < HISTOGRAM_WIDTH?

\$\endgroup\$
  • \$\begingroup\$ Correct. I'll use constants in the future. \$\endgroup\$ – cody.codes Sep 9 '15 at 22:20

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.