Print the frequency of characters as a horizontal histogram

Question

Well, I've used some of the feedback from my last posting to improve how I wrote this program. Mainly using the void parameter in main, initializing my int array to zero without using a for-loop, and better naming of variables. If there are some issues that are going on with my code, or there's some things I can do better, please let me know.

Any feedback is welcome!

#include <stdio.h>

/* print histogram of the frequencies of different characters in its input */
int main(void)
{
  int histogram[95] = {0}; /* initialize 95 spaces for ASCII characters 32 - 127 */
  int c, i, j;

  while ((c = getchar()) != EOF) {
    ++histogram[c-' '];
  }

  for (i = 32; i < 127; ++i) {
    printf("%c ", i);
    ++histogram[c-' '];
    for (j = 0; j < histogram[i-' ']; ++j) {
      putchar('x');
    }
    putchar('\n');
  } 
}

Answer 1

When dealing with offset arrrays (using position 0 to indicate character 32), it is important for readability to use symmetrical access patterns for the process. In your case, you load the values as c-' ', but what you use to read them, is accessed by i = 32; i < 127 .... and this asymmetrical access is inconvenient.

You also have no handling for out-of-bound characters.... can you trust your input sources to not have a carriage return, or a tab? Even esoteric null characters, or other binary values. I would have a catch-all 'bucket' for invalid values. That will prevent segfaults or other undefined actions.

You have a small bug in your code. In the print-loop, you increment the last character received.... what is this line for in the 'print' loops:

++histogram[c-' '];

Finally, for code reuse, it is often important to extract your functionality to a single reentrant method, other than the main method. Consider a signature like:

void histogram(const int offset, const int range)

where your histogram can be configured to take an offset position, and range, like:

histogram('a', 'z' - 'a' + 1);

which will count just the lower-case letters...

Something like:

#include <stdio.h>
#include <memory.h>

void histogram(const int offset, const int range) {
  int histogram[range];
  memset(histogram, 0, sizeof(histogram));

  int special = 0;

  int c;
  while ((c = getchar()) != EOF) {
    if (c < offset || c >= (offset + range)) {
        special++;
    } else {
        ++histogram[c - offset];
    }
  }

  for (int i = 0; i < range; ++i) {
    c = i + offset;
    printf("%c ", c);
    for (int j = 0; j < histogram[i]; ++j) {
      putchar('x');
    }
    putchar('\n');
  }

  printf("- ");
  for (int j = 0; j < special; j++) {
    putchar('x');
  }
  putchar('\n');
}

int main(void) {
    histogram(' ', 95);
}