K$R xrcise 1-13 (histogram)

Discussion in 'C Programming' started by dafrix, Feb 10, 2012.

  1. dafrix

    dafrix Guest

    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);
    }
     
    dafrix, Feb 10, 2012
    #1
    1. Advertising

  2. dafrix

    Ike Naar Guest

    On 2012-02-10, dafrix <> wrote:
    > 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.
     
    Ike Naar, Feb 11, 2012
    #2
    1. Advertising

  3. dafrix

    dafrix Guest

    On 11 velj, 01:16, Ike Naar <> wrote:
    > On 2012-02-10, dafrix <> wrote:
    >
    >
    >
    >
    >
    >
    >
    >
    >
    > >  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! :D
     
    dafrix, Feb 11, 2012
    #3
  4. dafrix <> writes:

    > 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.)

    --
    Ben.
     
    Ben Bacarisse, Feb 11, 2012
    #4
  5. dafrix

    dafrix Guest

    On 11 velj, 01:25, Ben Bacarisse <> wrote:
    > dafrix <> writes:
    > >  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.)
    >
    > --
    > Ben.


    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
     
    dafrix, Feb 11, 2012
    #5
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Oracle3001

    Building a Histogram using JAI

    Oracle3001, Oct 21, 2003, in forum: Java
    Replies:
    0
    Views:
    1,280
    Oracle3001
    Oct 21, 2003
  2. Jacky

    [Java Newbie] Histogram

    Jacky, Mar 2, 2005, in forum: Java
    Replies:
    2
    Views:
    614
  3. WreckingCru

    Word Histogram in C++??

    WreckingCru, Oct 16, 2003, in forum: C++
    Replies:
    7
    Views:
    2,935
    Alan Morgan
    Oct 23, 2003
  4. Engineer

    color histogram (help)

    Engineer, Mar 3, 2004, in forum: C++
    Replies:
    1
    Views:
    528
    Guybrush Threepwood
    Mar 3, 2004
  5. Davor Afrić

    K&R xrcise 1-13

    Davor Afrić, Feb 10, 2012, in forum: C++
    Replies:
    6
    Views:
    300
    Peter Remmers
    Feb 11, 2012
Loading...

Share This Page