Thread: Code Review Request: Number Histogram

  1. #1
    Registered User
    Join Date
    Jun 2018
    Posts
    2

    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:
    Code Review Request: Number Histogram-fnrrbqd-png
    Last edited by beef_erikson; 1 Week Ago at 03:09 PM.

  2. #2
    Registered User john.c's Avatar
    Join Date
    Dec 2017
    Posts
    291
    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;
    }
    Simplicity is the ultimate sophistication.

  3. #3
    Registered User
    Join Date
    Jun 2018
    Posts
    2
    Quote Originally Posted by john.c View Post
    Modern C allows the // single line comment.
    Ahhh wonderful. I was missing an easy single line comment. Noted!

    Quote Originally Posted by john.c View Post
    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!

    Quote Originally Posted by john.c View Post
    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.

    Quote Originally Posted by john.c View Post
    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. #4
    Registered User
    Join Date
    Apr 2017
    Location
    Quetzaltenango
    Posts
    75
    Quote Originally Posted by beef_erikson View Post
    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. #5
    Registered User
    Join Date
    Apr 2017
    Location
    Quetzaltenango
    Posts
    75
    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");
    Last edited by christophergray; 1 Week Ago at 03:15 PM.

  6. #6
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    25,861
    Quote 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.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  7. #7
    Registered User john.c's Avatar
    Join Date
    Dec 2017
    Posts
    291
    Quote Originally Posted by christophergray View Post
    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");
    Simplicity is the ultimate sophistication.

  8. #8
    Registered User
    Join Date
    Apr 2017
    Location
    Quetzaltenango
    Posts
    75
    Quote Originally Posted by laserlight View Post
    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. #9
    Registered User
    Join Date
    Nov 2010
    Location
    Long Beach, CA
    Posts
    5,904
    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.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Request for code review
    By thelastcylon in forum C++ Programming
    Replies: 4
    Last Post: 02-22-2017, 01:32 PM
  2. problem printing histogram about number frequencies
    By Hana Nasser in forum C Programming
    Replies: 14
    Last Post: 03-17-2015, 02:47 AM
  3. request for C code for factorial of any wish number
    By neverthought in forum C Programming
    Replies: 17
    Last Post: 05-28-2010, 06:27 AM
  4. code review request: stack implementation
    By cs32 in forum C Programming
    Replies: 6
    Last Post: 02-24-2008, 02:26 PM
  5. Request for peer review
    By ruthgrin in forum C++ Programming
    Replies: 13
    Last Post: 01-26-2006, 10:03 PM

Tags for this Thread