HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Print the length of words as input to a histogram with horizontal bars

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thewithlengthwordsinputhistogramprinthorizontalbars

Problem

It works, but I'm sure there's a lot of improvement I could make here. I didn't try to get it to work for words over 10 characters in length, and I also didn't want to use built in properties like .length to do the work for me (I don't know why, I just wanted to figure out how to do it on my own).

#include 

/* print the length of words as input to a histogram with horizontal bars */
int main() {
  int c, i, j;
  int accum = 0;
  int nchar[10];

  for (i = 0; i  0) {
    for (j = 0; j < nchar[i]; ++j) {
      putchar('x');
    }
  }
  putchar('\n');
  }
  return 0;
}

Solution

Overall this look pretty good. It's very straightforward, and easy to read. Here are a few things I'd work on improving:
Better Naming

I can live with c, i and j. Those are fairly reasonable short names. But I'd rename accum to length or word_len because what you're accumulating is the length of the next word. It's easier to read. Likewise, for nchar, I'd rename it to either histogram or num_chars (or numChars, or whatever your preference is).
Functions Separate Responsibility

Your code is doing 3 things:

  • Initializing your variables



  • Getting the words from the user (during which it calculates the histogram)



  • Displaying the histogram



I'd make at least 2 functions. Initializing the array is fine where it is for now. In the future, you might have more complex data structures you use to keep track of statistics, so you would probably create a initialization function for those.

You could have a function for calculating the histogram and another for displaying it. Something like:

void CalcHistogram(int histogram[10])
{
    int c;
    int length = 0;

    while ((c = getchar()) != EOF && length < 10) {
        if (c != ' ' && c != '\n' && c != '\t') {
            ++length;
        }
        else {
            ++histogram [ length ];
            length = 0;
        }
    }
}


and

void DisplayHistogram(const int histogram[10])
{
      for (int i = 0; i  0) {
            for (int j = 0; j < histogram [ i ]; ++j) {
                putchar('x');
            }
        }
        putchar('\n');
    }
}


As you can see, I also made the loop control variables local to the loop.

By separating the display into its own function, you can replace it with something more sophisticated later (like displaying it graphically, or instead sending it over the network, or whatever), without changing any of the logic of calculating it.
Error Handling

You aren't checking for errors at all. If the user enters a word longer than 10 characters, it suddenly stops taking input and prints out the histogram. That will be surprising to users. I'd at least output a message saying the input was too long, or something like that, if length (accum) gets over 10.

While I'm taking about user interface stuff, I notice that you don't prompt the user for what kind of input to enter. It might be worth adding a simple printf() to tell them what your program expects.

Code Snippets

void CalcHistogram(int histogram[10])
{
    int c;
    int length = 0;

    while ((c = getchar()) != EOF && length < 10) {
        if (c != ' ' && c != '\n' && c != '\t') {
            ++length;
        }
        else {
            ++histogram [ length ];
            length = 0;
        }
    }
}
void DisplayHistogram(const int histogram[10])
{
      for (int i = 0; i < 10; ++i) {
        
        printf("%d ", i);
        
        if (histogram [ i ] > 0) {
            for (int j = 0; j < histogram [ i ]; ++j) {
                putchar('x');
            }
        }
        putchar('\n');
    }
}

Context

StackExchange Code Review Q#103978, answer score: 3

Revisions (0)

No revisions yet.