# Thread: Code Review Request: Number Histogram

1. ## Code Review Request: Number Histogram

Hello! I've been wanting to learn C a while now and finally decided to pick up Kernighan and Ritchie's ANSI C. One of the 'projects' is to create a numbered histogram, which I successfully did.

I just would like to know if this could be further optimized or any other pointers you all may have.

Thanks!

Code:
```/* Histogram for counting input of numbers.
Troy Martin, Beef Erikson Studios 2018 */

#include <stdio.h>

int main()
{
/* forward declares */
int c, i, j;
int ndigit[10];

/* variable / array assignment */
int number = 0;
for (i = 0; i < 10; ++i)
ndigit[i] = 0;

/* reads input and puts numbers into corresponding array index */
while ((c = getchar()) != EOF)
if (c >= '0' && c <= '9')
++ndigit[c-'0'];
printf("\n\n");

/* finds highest number */
for (i = 0; i < 10; ++i)
if (ndigit[i] > number)
number = ndigit[i];

/* display histogram based on highest number */
for (i = 0; i < number; --number) {
for (j = 0; j < 10; ++j) {
if (ndigit[j] >= number)
printf("   *");
else
printf("    ");
}
printf("\n");
}

/* prints footer */
for (i = 0; i < 10; ++i)
printf("%4d", i);
printf("\n\n  Histogram of times each number was used.\n\n");
}```
Here it is running:

2. Modern C allows the // single line comment.

Modern C allows you to define your variables close to their first use, at which point they should also be initialized if possible. In particular, the index variable in for loops can be defined in the loop head.

Your loop to print the histogram doesn't use (or need) the variable i at all.

You should return 0 at the end (although modern C should do that for you if you leave it out).

A one-to-one graph is not necessarily the best. What if there are hundreds of some of the digits?
Code:
```// Histogram for counting input of digits.

#include <stdio.h>
#include <ctype.h>

int main() {
int ndigit[10] = {0};

// read input and put digits into corresponding array index
int c;
while ((c = getchar()) != EOF)
if (isdigit(c))
++ndigit[c - '0'];
printf("\n\n");

// find highest count
int high = 0;
for (int i = 0; i < 10; ++i)
if (ndigit[i] > high)
high = ndigit[i];

// display histogram based on highest count
while (high-- > 0) {
for (int j = 0; j < 10; ++j) {
printf("   ");
putchar(ndigit[j] > high ? '*' : ' ');
}
putchar('\n');
}

// print footer
for (int i = 0; i < 10; ++i)
printf("%4d", i);
printf("\n\n  Histogram of times each digit was used.\n\n");

return 0;
}```

3. Originally Posted by john.c
Modern C allows the // single line comment.
Ahhh wonderful. I was missing an easy single line comment. Noted!

Originally Posted by john.c
Modern C allows you to define your variables close to their first use, at which point they should also be initialized if possible. In particular, the index variable in for loops can be defined in the loop head.
Also noted and less of a pain in the ass, thank you!

Originally Posted by john.c
Your loop to print the histogram doesn't use (or need) the variable i at all.

You should return 0 at the end (although modern C should do that for you if you leave it out).
Thanks for pointing that out about i, I'll go back and fix. I also didn't know about returning 0; I'll be certain to do that.

Originally Posted by john.c
A one-to-one graph is not necessarily the best. What if there are hundreds of some of the digits?
Yeah, I wouldn't have chosen to do things this way myself but the example wanted me to to do it that way. Good point though and I agree!

Thanks a lot for taking the time to look over this... apparently modern C has quite a few changes I should be looking into. I took a look at the 'suggested books' thread; will probably pick up one of those as well as the one I'm using for a more 'modern' approach.

I appreciate it!!

4. Originally Posted by beef_erikson
I took a look at the 'suggested books' thread; will probably pick up one of those as well as the one I'm using for a more 'modern' approach.
K&R is still one of the best books to learn C. C really hasn't changed much. Wikipedia has an article on changes in C99.

5. I started putting curly braces in like this because I use a linter (splint) that only understands variable declarations at the start of a block. But it turns out that by reducing the scope of the variables declared, it makes the code easier to reason about. I can forget about the int c variable after I read past the right curly brace.
Code:
```// read input and put digits into corresponding array index
{ int c;
while ((c = getchar()) != EOF)
if (isdigit(c))
++ndigit[c - '0'];
}
printf("\n\n");```

6. Originally Posted by christophergray
I started putting curly braces in like this because I use a linter (splint) that only understands variable declarations at the start of a block. But it turns out that by reducing the scope of the variables declared, it makes the code easier to reason about. I can forget about the int c variable after I read past the right curly brace.
If you find yourself doing that, then perhaps it is time to move that block of code into its own function, possibly condensing the comment into a function name.

7. Originally Posted by christophergray
by reducing the scope of the variables declared, it makes the code easier to reason about.
I would normally write it like this but I didn't want to confuse the OP:
Code:
```    for (int c; (c = getchar()) != EOF; )
if (isdigit(c))
++ndigit[c - '0'];
printf("\n\n");```

8. Originally Posted by laserlight
If you find yourself doing that, then perhaps it is time to move that block of code into its own function, possibly condensing the comment into a function name.
Do you think it would be good style if the main() function for the above program consisted of nothing more than an array declaration and four function calls? Or is that going to far?

9. In general, that would be considered good style. I like when each function in my code reads like a summary in English of what it does. Obviously this doesn't apply to the inner-most functions.

In your case, I might consider three function calls. The names are just suggestions, but they have key elements of good style: they're clear, concise and descriptive.
Code:
```count_digits
find_highest_count
display_histogram```
Then, if you want to break up display_histogram into two functions (though it's probably not necessary in this case since it's so small, and so connected in purpose), you could have
Code:
```display_chart
display_footer```
Another aspect of good style is to keep terminology uniform. Your comments are "display histogram ..." and "print footer". If you decide to split them, use print_XXX for both or display_XXX for both since they do the same basic thing; don't mix usage of print and display. It can make readers of your code think there is a critical difference when there isn't.