K$R xrcise 1-13 (histogram)

[dafrix]

Hello there! I'm a newb to C and teaching myself from the K&R book,
going thru exercises. I was hoping somebody here would help me see the
mistake I'm missing in my code.
It seems to work, except that it runs the y-axis way up in the sky
for no reason, beyond the largest number being used in the program.
Sorry if this is an inapropriate post for usenet, I'm a newb here
also Here's the code:

#include <stdio.h>
#define IN 1
#define OUT 0

int max_array(int array[], int num_elements); // prototype of a
function
int sum_array(int array[], int num_elements); // prototype

main()
{
int words[11];
int i, j, c=0, c_count=0, w_count=0, pos=OUT;
for(i = 0; i <= 10; ++i)
words = 0;

while ((c = getchar()) != EOF)
{
if(c==' ' || c=='\n' || c=='\t')
pos = OUT;
else if(pos == OUT)
pos = IN;
if(pos == IN)
++c_count;
else if(c_count <= 10)
{
++words[c_count];
++w_count;
c_count = 0;
}
else
{
++w_count;
c_count = 0;
}
}
w_count = w_count - (sum_array(words,11));

// draw the histogram
for(j = (max_array(words, 11)); j >= 1; --j)
{
printf(" %4d|", j);
for(i = 1; i < 11; ++i)
{
if(words >= j)
printf(" *");
else
printf(" ");
}
if(w_count >= j)
printf(" *");
else
printf(" ");
putchar ('\n');
}
for(i = 0; i < 51; ++i) printf("_");
putchar('\n');
printf(" ");
for(i = 1; i < 11; ++i) printf("%4d", i);
printf(" >10\n");
return 0;
}

// function to return the largest integer in an array
int max_array(int array[], int num_elements)
{
int i, max=-32000;
for(i = 0; i < num_elements; ++i)
{
if(array > max)
max = array;
}
return (max);
}

// function to return the sum of an array's elements
int sum_array(int array[], int num_elements)
{
int i, sum=0;
for(i = 0; i < num_elements; ++i)
sum = sum + array;
return (sum);
}

 

[Ike Naar]

Hello there! I'm a newb to C and teaching myself from the K&R book,
going thru exercises. I was hoping somebody here would help me see the
mistake I'm missing in my code.
It seems to work, except that it runs the y-axis way up in the sky
for no reason, beyond the largest number being used in the program.
Sorry if this is an inapropriate post for usenet, I'm a newb here
also Here's the code:

[snip]

[inconsistent indendation fixed]
if(c==' ' || c=='\n' || c=='\t')
pos = OUT;
else if(pos == OUT)
pos = IN;
if(pos == IN)
++c_count;
else if(c_count <= 10)
{

This branch is reached then pos == OUT and c_count <= 10.
Note that c_count can be (and often is) 0 here.
As a result words[0] gets a high count.

++words[c_count];
++w_count;
c_count = 0;
}
else
{
++w_count;
c_count = 0;
}
}

The high count of words[0] is responsible for a high value of
max_array(words,11) thet is used when printing the histogram.

 

[dafrix]

 Hello there! I'm a newb to C and teaching myself from the K&R book,
going thru exercises. I was hoping somebody here would help me see the
mistake I'm missing in my code.
 It seems to work, except that it runs the y-axis way up in the sky
for no reason, beyond the largest number being used in the program.
Sorry if this is an inapropriate post for usenet, I'm a newb here
also Here's the code:

[snip]

[inconsistent indendation fixed]
        if(c==' ' || c=='\n' || c=='\t')
            pos = OUT;
        else if(pos == OUT)
            pos = IN;
        if(pos == IN)
            ++c_count;
        else if(c_count <= 10)
        {

              This branch is reached then pos == OUT and c_count <= 10.
              Note that c_count can be (and often is) 0 here.
              As a result words[0] gets a high count.

            ++words[c_count];
            ++w_count;
            c_count = 0;
        }
        else
        {
            ++w_count;
            c_count = 0;
        }
    }

The high count of words[0] is responsible for a high value of
max_array(words,11) thet is used when printing the histogram.

That must be true! Thamk you very much!

 

[Ben Bacarisse]

Hello there! I'm a newb to C and teaching myself from the K&R book,
going thru exercises. I was hoping somebody here would help me see the
mistake I'm missing in my code.
It seems to work, except that it runs the y-axis way up in the sky
for no reason, beyond the largest number being used in the program.
Sorry if this is an inapropriate post for usenet, I'm a newb here
also Here's the code:

First up: well done. Good solution. If I have a lot of tings to say,
it is because I like to type, not because there's very much wrong with
your code.

#include <stdio.h>
#define IN 1
#define OUT 0

int max_array(int array[], int num_elements); // prototype of a
function
int sum_array(int array[], int num_elements); // prototype

Try to avoid obvious comments. Your functions have good names and clear
parameters, so no comment is really needed. There is a comment you
could give about max_array, but see later for more on that.

main()

int main(void)

{
int words[11];
int i, j, c=0, c_count=0, w_count=0, pos=OUT;
for(i = 0; i <= 10; ++i)
words = 0;

I'd name the size so as to avoid having repeated (or related) number in
several places. In general, if you use something twice, name it; if you
use something once, consider naming it!

while ((c = getchar()) != EOF)
{
if(c==' ' || c=='\n' || c=='\t')
pos = OUT;
else if(pos == OUT)
pos = IN;

You posted code with a mix of tab and spaces. This almost always goes
wrong on Usenet because different software treats tabs differently.
I've edited it to correct the layout.

if(pos == IN)
++c_count;
else if(c_count <= 10)

Here you could use < MAX_WORD_SIZE if you'd named it.

And here's the problem you asked about: when POS == OUT you keep
counting zero-length words. At least this is one place you could fix
the behaviour. Another is below when you draw the histogram. You could
simply ignore words[0] in the calculation of the maximum, but that's a
little more fiddly.

{
++words[c_count];
++w_count;
c_count = 0;
}
else
{
++w_count;
c_count = 0;
}
}
w_count = w_count - (sum_array(words,11));

Why the ()s?

// draw the histogram
for(j = (max_array(words, 11)); j >= 1; --j)

.... and there's another 11!

The other place to fix the drawing is here. If you write

for (j = max_array(words + 1, 10); j >= 1; --j)

you'll get the maximum of words[1] to words[10] -- ignoring words[0].
You may not yet have covered the part of C that makes this work, so you
may be better off fixing it above.

You've used good names so far, so you could have picked something better
than j. OK, it is just a counter, but the longer the loop body the
longer the loop counter name should be. It's counting down frequencies
of words, so maybe "word_frequency"?

{
printf(" %4d|", j);
for(i = 1; i < 11; ++i)

.... oh, and another! (I think you get the point.)

{
if(words >= j)
printf(" *");
else
printf(" ");
}
if(w_count >= j)
printf(" *");
else
printf(" ");
putchar ('\n');
}
for(i = 0; i < 51; ++i) printf("_");

Even this 51 is related to the 11 above.

putchar('\n');
printf(" ");
for(i = 1; i < 11; ++i) printf("%4d", i);
printf(" >10\n");
return 0;
}

// function to return the largest integer in an array
int max_array(int array[], int num_elements)
{
int i, max=-32000;
for(i = 0; i < num_elements; ++i)
{
if(array > max)
max = array;
}
return (max);
}

This works, but it's always a little ugly to invent an artificial non
maximum. If you have to, use INT_MIN after #include <limits.h>, but I'd
write

int max_array(int array[], int num_elements)
{
int max = arrya[0];
for (int i = 1; i < num_elements; ++i)
if (array > max)
max = array;
return max;
}

Note I'm using C99's for-loop syntax. You are using C99 comments
(//....) so I figured, why not. K&R never write a C99 update to TCPL
(and now they never will since Ritchie died later year).

// function to return the sum of an array's elements
int sum_array(int array[], int num_elements)
{
int i, sum=0;
for(i = 0; i < num_elements; ++i)
sum = sum + array;
return (sum);
}

It's a little more C-ish to write sum += array and most people don't
put () in a return statement. (It used to be required, but that was
before even the first edition of K&R.)

 

[dafrix]

 Hello there! I'm a newb to C and teaching myself from the K&R book,
going thru exercises. I was hoping somebody here would help me see the
mistake I'm missing in my code.
 It seems to work, except that it runs the y-axis way up in the sky
for no reason, beyond the largest number being used in the program.
Sorry if this is an inapropriate post for usenet, I'm a newb here
also Here's the code:

First up: well done.  Good solution.  If I have a lot of tings to say,
it is because I like to type, not because there's very much wrong with
your code.

#include <stdio.h>
#define IN 1
#define OUT 0

int max_array(int array[], int num_elements);    // prototype of a
function
int sum_array(int array[], int num_elements);    // prototype

Try to avoid obvious comments.  Your functions have good names and clear
parameters, so no comment is really needed.  There is a comment you
could give about max_array, but see later for more on that.

main()

int main(void)

{
    int words[11];
    int i, j, c=0, c_count=0, w_count=0, pos=OUT;
    for(i = 0; i <= 10; ++i)
        words = 0;

I'd name the size so as to avoid having repeated (or related) number in
several places.  In general, if you use something twice, name it; if you
use something once, consider naming it!

    while ((c = getchar()) != EOF)
    {
        if(c==' ' || c=='\n' || c=='\t')
            pos = OUT;
        else if(pos == OUT)
            pos = IN;

You posted code with a mix of tab and spaces.  This almost always goes
wrong on Usenet because different software treats tabs differently.
I've edited it to correct the layout.

        if(pos == IN)
            ++c_count;
        else if(c_count <= 10)

Here you could use < MAX_WORD_SIZE if you'd named it.

And here's the problem you asked about: when POS == OUT you keep
counting zero-length words.  At least this is one place you could fix
the behaviour.  Another is below when you draw the histogram.  You could
simply ignore words[0] in the calculation of the maximum, but that's a
little more fiddly.

                {
                     ++words[c_count];
                     ++w_count;
                     c_count = 0;
                }
        else
                {
                     ++w_count;
                     c_count = 0;
                }
    }
    w_count = w_count - (sum_array(words,11));

Why the ()s?

    // draw the histogram
    for(j = (max_array(words, 11)); j >= 1; --j)

... and there's another 11!

The other place to fix the drawing is here.  If you write

  for (j = max_array(words + 1, 10); j >= 1; --j)

you'll get the maximum of words[1] to words[10] -- ignoring words[0].
You may not yet have covered the part of C that makes this work, so you
may be better off fixing it above.

You've used good names so far, so you could have picked something better
than j.  OK, it is just a counter, but the longer the loop body the
longer the loop counter name should be.  It's counting down frequencies
of words, so maybe "word_frequency"?

    {
        printf(" %4d|", j);
        for(i = 1; i < 11; ++i)

... oh, and another!  (I think you get the point.)

        {
            if(words >= j)
                printf("   *");
            else
                printf("    ");
        }
        if(w_count >= j)
                printf("   *");
        else
                printf("    ");
        putchar ('\n');
    }
    for(i = 0; i < 51; ++i) printf("_");

Even this 51 is related to the 11 above.

    putchar('\n');
    printf("      ");
    for(i = 1; i < 11; ++i) printf("%4d", i);
    printf(" >10\n");
    return 0;
}

// function to return the largest integer in an array
int max_array(int array[], int num_elements)
{
        int i, max=-32000;
        for(i = 0; i < num_elements; ++i)
                {
                if(array > max)
                        max = array;
                }
        return (max);
}

This works, but it's always a little ugly to invent an artificial non
maximum.  If you have to, use INT_MIN after #include <limits.h>, but I'd
write

  int max_array(int array[], int num_elements)
  {
      int max = arrya[0];
      for (int i = 1; i < num_elements; ++i)
          if (array > max)
              max = array;
      return max;
  }

Note I'm using C99's for-loop syntax.  You are using C99 comments
(//....) so I figured, why not.  K&R never write a C99 update to TCPL
(and now they never will since Ritchie died later year).

// function to return the sum of an array's elements
int sum_array(int array[], int num_elements)
{
    int i, sum=0;
    for(i = 0; i < num_elements; ++i)
        sum = sum + array;
    return (sum);
}

It's a little more C-ish to write sum += array and most people don't
put () in a return statement.  (It used to be required, but that was
before even the first edition of K&R.)

Thank you Ben for your points, I'm sure to pay attention to these
things now that you
mentioned. As it is obvious I still haven't developed typing "hygiene"
and your feedback
means a lot!
The braces () got there in one of my attempts to find and fix a bug
somewhere
along the line, which I should probably have saved as another version
of the file btw, just
to keep tidy. I think I'd like to have a variable with number of
spaces just in case and
so to me it seems like a good idea to edit the max_array function so
as not to return
space count when figuring out the maximum.
Cheers,

Davor