4
\$\begingroup\$

This is for K&R C Exercise 1-14: basically asking to create a program that prints a histogram of the frequency of types of characters. This is done with very basic C functions/knowledge as covered in the first chapter.

I feel like this code as it stands is pretty clunky with all the for loops to print (though it does function!) Is there a simple way to reduce the number of loops (and also the loop variables) in a more concise way?

#include <stdio.h>

int main()
{
    int c, i, j, k, l, h, nwhite, nother;
    int ndigits[10];

    nwhite = nother = 0;

    for (i = 0; i < 10; ++i)
        ndigits[i] = 0;

    while ((c = getchar()) != EOF)
    {
        if (c >= '0' && c <= '9')
            ++ndigits[c-'0'];
        else if (c == '\n' || c == '\t' || c == ' ')
            ++nwhite;
        else 
            ++nother;
    }
    for (j = 0; j <= nwhite; ++j)
    {
        if (nwhite - j > 0)
            putchar('*');
    }
    putchar('\n');
    for (k = 0; k <= nother; ++k)
    {
        if (nother - k > 0)
            putchar('*');
    }
    putchar('\n');
    for (i = 0; i < 10; ++i)
    {
        for (h = 0; h < ndigits[i]; ++h)
        {
            if (ndigits[i] - h > 0)
                putchar('*');
        }
        putchar('\n');
    }
}
\$\endgroup\$
4
\$\begingroup\$

Nice job on your code. I only have a few things to say along with some extra nit-picks at the end.


Refactor

I see this construct a lot in your code:

for (j = 0; j <= nwhite; ++j)
{
    if (nwhite - j > 0)
        putchar('*');
}
putchar("\n");

Of course, different variables are used every time, but the concept is the same. You could actually simplify your code if you extracted this logic to a separate function. Here is what I mean:

void print_stars(limit) {
    for(int i = 0; i < limit; i++) {
        if(limit - i > 0) {
            putchar("*");
        }
    }
    putchar("\n");
}

Now, you can just use this function every time you come across this construct:

print_stars(nwhite);
print_stars(nother);
for (i = 0; i < 10; ++i)
{
    print_stars(ndigits[i]);
}

Nitpicks

That refactoring was really all I had to say about your code. These are just extra (yet important) things.

  • Always use braces. I notice that you've omitted braces on some if statements, and that's probably because K&R does that a lot. In actual code, this is bad practice because it can lead to hard-to-find yet easy-to-get bugs.
  • You can simplify initializing arrays with 0s with this:

    int ndigits[10] = {0};
    

There is no need for an extra loop to do this.

\$\endgroup\$
  • \$\begingroup\$ Thank you! Functions are actually covered in the very next section, so that was very helpful practice. \$\endgroup\$ – Jonathan Scott Mar 1 '16 at 18:22
2
\$\begingroup\$

Definitions

K&R routinely defines multiple variables in a single line, but it's now pretty widely accepted that it's generally better to define each variable individually, or at least format the definition with one variable per line, so you'd have either:

int c, 
    i, 
    j, 
    k, 
    l, 
    h, 
    nwhite, 
    nother;

or:

int c;
int i;
int j;
// ...
int nother;

Use standard library

Although you probably haven't been introduced to them at this point in the book, the C standard library has isdigit and isspace functions (in <ctype.h>) that are generally preferred for testing whether a character is a digit or space character respectively.

Data structures:

I'd at least consider combining nwhite, nother and ndigits into a single array, with an enum to specify indices into that array:

enum { WHITE, OTHER, ZERO, ONE, TWO, THREE, FOUR, FIVE, SIX, SEVEN, EIGHT, NINE, LAST };

int counts[LAST];

while (EOF != (ch=getchar())) {
    if (isdigit(ch))
        ++counts[ZERO + ch-'0'];
    else if (isspace(ch))
        ++counts[WHITE];
    else
       ++counts[OTHER];
}

Then you can print out all the values with a single loop:

for (i=0; i<LAST; i++) {
    for (j=0; j<counts[i]; j++) {
        putchar('*');
    }
    putchar('\n');
}

Appropriate display

Although it's way outside the scope of the exercise, if I were going to put this to real use, I'd at least consider normalizing the results, so the longest is always (for example) 79 characters, and the others are scaled proportionally to that. As it stands right now, it only really works well if the count for each type of input fits in a single line of output. You can still get by if a few produce a couple lines each, but try to run it on the text of a large book, and each count will produce many lines of output, and to get a comparison you'd need to count them manually.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.