5
\$\begingroup\$

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');
  } 
}
\$\endgroup\$
2
\$\begingroup\$

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);
}
\$\endgroup\$
  • \$\begingroup\$ Ok, so you ensure that the input is always going to be plotted in the histogram by assigning the characters which aren't in the range of the offset to special, then plotting those after the printf("- "); at the end? I'm sure that this is an easier method than say, trying to get the input of every possible ASCII value... but is it possible then to catch all the ASCII values if I didn't want to use special to plot a histogram? Is there a function that can do that in stdio.h? Thanks! \$\endgroup\$ – cody.codes Sep 7 '15 at 22:32
  • 1
    \$\begingroup\$ @cody.codes: look at the various isfoo functions in <ctypes.h>, e.g. isalpha. (All 7-bit values are ASCII, including the control codes and non-printable chars.) \$\endgroup\$ – Mat Sep 8 '15 at 4:46

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.