patterncMinor
Print the length of words as input to a histogram with horizontal bars
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
Functions Separate Responsibility
Your code is doing 3 things:
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:
and
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
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
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.