Thread: 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:

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;
}

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");

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.

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");