4
\$\begingroup\$

From The C Programming Language, by Kernighan and Ritchie:

Exercise 1-13. Write a program to print a histogram of the lengths of words in its input. It is easy to draw the histogram with the bars horizontal; a vertical orientation is more challenging. (Kernighan, 24)

This is my first time learning C seriously. I've started the few first pages over a thousand times, but now I've decided to get down to it and do the book from A to Z. My code for Exercise 1-13 is as follows:

#include<stdio.h>

#define MAXWLENGTH  20
#define MAXWNO      20
#define OUT         0
#define IN          1
#define NONBLANK    '-'

int main(void){
    int wrdlength[MAXWNO], wrdid, longestwrd, wrdno;
    int current, last; // current and before-current characters being processed
    int i, ii; // for loop counters
    int state; // determines whether inside or outside of current word

    for(i = 0; i < MAXWNO; i++)
        wrdlength[i] = 0; // (tested and works)

    last = NONBLANK;
    state = OUT;
    wrdid = 0; // range 0–19
    i = 0;
    longestwrd = 0;

    // collect data from input // tested and works
    while(((current = getchar()) != EOF) && (wrdid < MAXWNO)){
        if(current == ' ' || current == '\n' || current == '\t'){
            if(last == ' ' || last == '\n' || last == '\t'){
                ; // STATE is already OUT
            }
            else{
                state = OUT;
                wrdid++; // jump to next word
            }
        }
        else{ // if nonblank character…
            if(state == OUT)
                state = IN;
            wrdlength[wrdid] = wrdlength[wrdid] + 1; // add letter to current word
        }
        last = current;
    }
    wrdno = wrdid; // get # of non-void words

    // find greatest array element (longest word) // tested and works
    for(i = 0, wrdid = 0; i < MAXWNO; i++)
        if(longestwrd < wrdlength[wrdid])
            longestwrd = wrdlength[wrdid];

    printf("\n\n\n\n--- Word Histogram ---\n\n");

    for(i = longestwrd; i > 0; i--){
        for(ii = 0; ii < wrdno; ii++)
            if(wrdlength[ii] >= i)
                printf("*"); // Unicode 25A0
            else // -> if(wrdlength[ii] < i)
                putchar(' '); // put space
        if(i != 1) // -> if(i > 1)
            printf("\n");
    }

    /*
    // this for loop simply prints out ALL the word lengths (even when zero)
    for(i = 0; i < MAXWNO; i++)
        printf("%d\n", wrdlength[i]);
    */
}

The comments are my personal remarks, and aren't aimed at flagging anything to seize a reviewer's attention. They were simply used to help me in the process of making the code.

My specific expectations from anyone who'd have the kindness to review my code: I'm looking for efficiency, readability, shortness (I know short is not necessarily efficient, and that's why I'm putting it after efficiency), and simply handling the problem in a way that is considered natural and intelligent to an experienced programmer's eyes.

I feel like this code could be shorter and more concise, but I'm stuck as to how I should do that. Again, I'm very new to C, and have never programed seriously in another language before.

Here's a demo output for my program:

INPUT:

Hello, world! How are you, today?
Since I have just come to birth, I know nothing.
And since I can learn nothing, I will know nothing more.

OUTPUT:





--- Word Histogram ---

**   *      *  *    
**   **     *  * *  
**  *** *** * ** *  
******* *** * **** *
******* ***** **** *
********************

Please take into account that anything that wasn't mentioned in the book up to page 24, I most likely do not know. Also, I'm aware the book I'm using is somewhat dated, so don't hesitate in suggesting techniques that might apply to more recent standards of C.

I'm compiling my code using the GCC, which was installed on Debian GNU/Linux 11 (bullseye) x86_64 via the official Debian repos.

References:

  1. Kernighan, B.W. and Ritchie, D.M. (1988). The C programming language / ANSI C Version. Englewood Cliffs, N.J.: Prentice Hall.
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Initialize at declaration

Rather than later zero the array, do so at declaration time.

//int wrdlength[MAXWNO];
...
//for(i = 0; i < MAXWNO; i++)
//    wrdlength[i] = 0; // (tested and works)

int wrdlength[MAXWNO] = { 0 };

Use isspace() to test for white-space

Instead of testing of some of the possible white-spaces, test for all of them. See <ctype.h>.

// if(current == ' ' || current == '\n' || current == '\t'){
if (isspace(current)) {

Style: Use {}

Even for one-liners, IMO, use {}.

        //if(state == OUT)
        //    state = IN;
        if(state == OUT) {
            state = IN;
        }

++

// wrdlength[wrdid] = wrdlength[wrdid] + 1;
wrdlength[wrdid]++;
// or 
++wrdlength[wrdid];

Bug

Loop body never uses i. Certainly incorrect.

for(i = 0, wrdid = 0; i < MAXWNO; i++)
    if(longestwrd < wrdlength[wrdid])
        longestwrd = wrdlength[wrdid];

Why is "*" commented as Unicode 25A0?

// ????
printf("*"); // Unicode 25A0

Test limit first

In this code, it makes no difference, yet in general, don't read from stdin unless code intends to do something with the input.

// while(((current = getchar()) != EOF) && (wrdid < MAXWNO)){
while((wrdid < MAXWNO) && ((current = getchar()) != EOF)){

Correct word ID count??

wrdid++; increments after a "blank" is found. If the entire input was "test", wrdid would remain at 0 whereas 1 is expected.

Instead I recommend incrementing wrdid when the first letter of a word is found.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Very nice review. I'm very thankful that you took the time to help me out. As for your very last remark, regarding the positioning or the wrdid increment, entering "test" will result in wrdid == 1 being true, because a newline has to be entered after a string for the input to be considered anyways. At least, that's my understanding of it. If an EOF signal is sent before a newline is entered, the program will not consider any of the words in the last line. (On my machine and environment, at least.) Anyhow, thank you so much again for reviewing my code. it's appreciated. God bless you. \$\endgroup\$
    – GPWR
    Dec 13, 2022 at 22:21
  • 1
    \$\begingroup\$ @GPWR "because a newline has to be entered after a string for the input to be considered anyways" --> It is commonly occurs, yet it is not true as the only way. An easy way to test is to redirect input from a file to stdin. \$\endgroup\$ Dec 13, 2022 at 22:27
  • \$\begingroup\$ Oh, I see. Thank you for clarifying that for me! It helps a lot. Peace. \$\endgroup\$
    – GPWR
    Dec 13, 2022 at 23:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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