5
\$\begingroup\$

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 <stdio.h>

/* 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 < 10; ++i) 
    nchar[i] = 0;

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

  for (i = 0; i < 10; ++i) {
    printf("%d ", i);
  if (nchar[i] > 0) {
    for (j = 0; j < nchar[i]; ++j) {
      putchar('x');
    }
  }
  putchar('\n');
  }
  return 0;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ "I also didn't want to use built in properties like .length to do the work for me" -- C doesn't have .length as Java and some other languages do. \$\endgroup\$ – Spikatrix Sep 7 '15 at 10:33
  • \$\begingroup\$ I wrote some code that accomplishes a task similar to yours in Ruby, you may take a look at it for a different approach to a similar problem. \$\endgroup\$ – Caridorc Sep 7 '15 at 11:02
  • \$\begingroup\$ Follow-up question \$\endgroup\$ – 200_success Sep 7 '15 at 21:19
3
\$\begingroup\$

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:

  1. Initializing your variables
  2. Getting the words from the user (during which it calculates the histogram)
  3. 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 < 10; ++i) {

        printf("%d ", i);

        if (histogram [ 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.

\$\endgroup\$
3
\$\begingroup\$

You said:

I didn't try to get it to work for words over 10 characters in length

However, your code does not correctly handle words that are even 10 characters long, let alone over 10:

$ ./main
1234567890
0
1
2
3
4
5
6
7
8
9

If you just want to handle words that are maximum 9 characters in length you don't need to allocate a 10-elements array because in such case one element is always unused, an array of 9 elements is enough. You just to need change the while loop condition.

You also said:

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

Of course, I am pretty sure that you know that in C you cannot do string.length to get a size of a string or an array, right?

Last thing is int main() part. In modern C it always should be int main(void). See this: https://stackoverflow.com/questions/29190986/is-int-main-without-void-valid-and-portable-in-iso-c

Besides that, I agree with everything user1118321 said in his answer.

\$\endgroup\$
1
\$\begingroup\$

This:

  for (i = 0; i < 10; ++i) {
    printf("%d ", i);
  if (nchar[i] > 0) {
    for (j = 0; j < nchar[i]; ++j) {
      putchar('x');
    }
  }
  putchar('\n');
  }

has some awkward indentation. It should be

  for (i = 0; i < 10; ++i) {
    printf("%d ", i);
    if (nchar[i] > 0) {
      for (j = 0; j < nchar[i]; ++j) {
        putchar('x');
      }
    }
    putchar('\n');
  }

Also, the if statement is unnecessary. Remove it:

  for (i = 0; i < 10; ++i) {
    printf("%d ", i);
    for (j = 0; j < nchar[i]; ++j) {
      putchar('x');
    }
    putchar('\n');
  }

And instead of declaring and then assigning:

  int nchar[10];

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

Initialize the array:

  int nchar[10] = {0};

which will also shorten the code.

FYI, the return statement at the end of main can be omitted as it is implicit in C99 and C11.

\$\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.