need some help with this histogram of words program........

Discussion in 'C Programming' started by Ceriousmall, Feb 2, 2011.

  1. Ceriousmall

    Ceriousmall Guest

    This is one of the programs from the K&R Ansi C Book that you have to
    write I've had a go at it but got kinda stumped here. So here's my
    effort so far...... And I'm aware there is a new ISO standard for C
    C99 as of March 2000 and I'm getting around to learning and adopting
    that as well but my main concern is actually learning the
    language....

    /* copyright 2011 Ceriousmall. . . .

    Program prints a histogram of the length of words in its input */

    #include <stdio.h>

    #define IN 1 /* inside a word */
    #define OUT 0 /* outside a word */

    main()
    {
    char x;
    int c, i, n, nchar, state;
    int wordlength[13];
    int horscale[13], verscale[51];

    x = '*';
    state = OUT;
    n = nchar = 0;

    for (i = 0; i < 13; ++i) {
    wordlength = 0;
    horscale = i;
    }
    for (i = 0; i < 51; ++i)
    verscale = i;

    while ((c=getchar()) != EOF) {
    if (c == ' ' || c == '\n' || c == '\t')
    state = OUT;
    else if (state == OUT) /* condition controlls the start */
    state = IN; /* of a word */

    if (state == IN)
    ++nchar;
    else if (nchar == 1)
    ++wordlength[1];
    else if (nchar == 2)
    ++wordlength[2];
    else if (nchar == 3)
    ++wordlength[3];
    else if (nchar == 4)
    ++wordlength[4];
    else if (nchar == 5)
    ++wordlength[5];
    else if (nchar == 6)
    ++wordlength[6];
    if (state == OUT)
    nchar = 0;
    }
    for (i = 50; i >= 0; --i)
    if (wordlength[1] == verscale)
    for (i = verscale; i >= 0; --i)
    printf("%2d| %c\n", verscale, x);
    printf(" +------------------------------------\n");

    for (i = 0; i < 13; ++i)
    printf("%3d", horscale);
    printf("\n");

    return 0;
    }
     
    Ceriousmall, Feb 2, 2011
    #1
    1. Advertising

  2. On Feb 2, 5:35 am, Ceriousmall <> wrote:
    >
    >                 if (state == IN)
    >                         ++nchar;
    >                 else if (nchar == 1)
    >                         ++wordlength[1];
    >                 else if (nchar == 2)
    >                         ++wordlength[2];
    >                 else if (nchar == 3)
    >                         ++wordlength[3];
    >                 else if (nchar == 4)
    >                         ++wordlength[4];
    >                 else if (nchar == 5)
    >                         ++wordlength[5];
    >                 else if (nchar == 6)
    >                         ++wordlength[6];
    >

    You can index by a variable. In fact this is the normal way to use
    arrays.

    So ++wordlength[nchar];

    (You also need to test nchar to make sure it doesn't go above 12 if
    you have a 13-element array. Remember the indexes count from zero.)
     
    Malcolm McLean, Feb 2, 2011
    #2
    1. Advertising

  3. Ceriousmall <> writes:

    > This is one of the programs from the K&R Ansi C Book that you have to
    > write I've had a go at it but got kinda stumped here. So here's my
    > effort so far...... And I'm aware there is a new ISO standard for C
    > C99 as of March 2000 and I'm getting around to learning and adopting
    > that as well but my main concern is actually learning the
    > language....
    >
    > /* copyright 2011 Ceriousmall. . . .
    >
    > Program prints a histogram of the length of words in its input */
    >
    > #include <stdio.h>
    >
    > #define IN 1 /* inside a word */
    > #define OUT 0 /* outside a word */
    >
    > main()
    > {
    > char x;
    > int c, i, n, nchar, state;
    > int wordlength[13];
    > int horscale[13], verscale[51];


    These "magic numbers" should be given names with a #define or an enum.

    > x = '*';
    > state = OUT;


    When there are only two states I refer to avoid neutral names like
    'state' in favour of boolean variables (in C90 these are just int
    variables) with a biased name: 'in_a_word' or 'between_words' and so
    on. That way, you don't need to name the states.

    > n = nchar = 0;
    >
    > for (i = 0; i < 13; ++i) {
    > wordlength = 0;
    > horscale = i;
    > }
    > for (i = 0; i < 51; ++i)
    > verscale = i;


    You set verscale to i and never change it. That's not a useful
    array -- you can always just use the index instead; but then you also
    don't use horscale in any useful way. Without any idea what these
    array are there for I can't suggest what you should be doing with them.
    You don't need either for a simple histogram with horizontal bars.

    > while ((c=getchar()) != EOF) {
    > if (c == ' ' || c == '\n' || c == '\t')
    > state = OUT;
    > else if (state == OUT) /* condition controlls the start */
    > state = IN; /* of a word */


    Look at the last if a bit more. What happens when state is IN? The
    effect will be to leave state as IN so you could just have written

    if (c == ' ' || c == '\n' || c == '\t')
    state = OUT;
    else state = IN;

    or even

    state = (c == ' ' || c == '\n' || c == '\t') ? OUT : IN;

    If you take my idea of a boolean rather than a neutral state variable,
    you could write:

    between_words = (c == ' ' || c == '\n' || c == '\t');

    >
    > if (state == IN)
    > ++nchar;
    > else if (nchar == 1)
    > ++wordlength[1];
    > else if (nchar == 2)
    > ++wordlength[2];
    > else if (nchar == 3)
    > ++wordlength[3];
    > else if (nchar == 4)
    > ++wordlength[4];
    > else if (nchar == 5)
    > ++wordlength[5];
    > else if (nchar == 6)
    > ++wordlength[6];


    As has been pointed out, you can write ++wordlength[nchar] (provided you
    test that nchar is not too big).

    > if (state == OUT)
    > nchar = 0;
    > }
    > for (i = 50; i >= 0; --i)
    > if (wordlength[1] == verscale)
    > for (i = verscale; i >= 0; --i)
    > printf("%2d| %c\n", verscale, x);
    > printf(" +------------------------------------\n");
    >
    > for (i = 0; i < 13; ++i)
    > printf("%3d", horscale);
    > printf("\n");


    This is all rather confused and I can't see what the objective is. If
    you say what you are trying to do here, it would help.

    >
    > return 0;
    > }


    --
    Ben.
     
    Ben Bacarisse, Feb 2, 2011
    #3
  4. Ceriousmall

    Ceriousmall Guest

    On Feb 2, 10:01 am, Ben Bacarisse <> wrote:
    > Ceriousmall <> writes:
    > > This is one of the programs from the K&R Ansi C Book that you have to
    > > write I've had a go at it but got kinda stumped here. So here's my
    > > effort so far...... And I'm aware there is a new ISO standard  for C
    > > C99 as of  March 2000 and I'm getting around to learning and adopting
    > > that as well but my  main concern is actually learning the
    > > language....

    >
    > > /* copyright 2011 Ceriousmall. . . .

    >
    > >    Program prints a histogram of the length of words in its input */

    >
    > > #include <stdio.h>

    >
    > > #define IN  1      /* inside a word */
    > > #define OUT 0      /* outside a word */

    >
    > > main()
    > > {
    > >    char x;
    > >    int c, i, n, nchar, state;
    > >         int wordlength[13];
    > >    int horscale[13], verscale[51];

    >
    > These "magic numbers" should be given names with a #define or an enum.
    >
    > >    x = '*';
    > >    state = OUT;

    >
    > When there are only two states I refer to avoid neutral names like
    > 'state' in favour of boolean variables (in C90 these are just int
    > variables) with a biased name: 'in_a_word' or 'between_words' and so
    > on.  That way, you don't need to name the states.
    >
    > >    n = nchar = 0;

    >
    > >    for (i = 0; i < 13; ++i) {
    > >            wordlength = 0;
    > >            horscale = i;
    > >    }
    > >    for (i = 0; i < 51; ++i)
    > >            verscale = i;

    >
    > You set verscale to i and never change it.  That's not a useful
    > array -- you can always just use the index instead; but then you also
    > don't use horscale in any useful way.  Without any idea what these
    > array are there for I can't suggest what you should be doing with them.
    > You don't need either for a simple histogram with horizontal bars.
    >
    > >    while ((c=getchar()) != EOF) {
    > >            if (c == ' ' || c == '\n' || c == '\t')
    > >                    state = OUT;
    > >            else if (state == OUT)   /* condition controlls the start */
    > >                    state = IN;      /* of a word */

    >
    > Look at the last if a bit more.  What happens when state is IN?  The
    > effect will be to leave state as IN so you could just have written
    >
    >    if (c == ' ' || c == '\n' || c == '\t')
    >         state = OUT;
    >    else state = IN;
    >
    > or even
    >
    >   state = (c == ' ' || c == '\n' || c == '\t') ? OUT : IN;
    >
    > If you take my idea of a boolean rather than a neutral state variable,
    > you could write:
    >
    >   between_words = (c == ' ' || c == '\n' || c == '\t');
    >
    >
    >
    > >            if (state == IN)
    > >                    ++nchar;
    > >            else if (nchar == 1)
    > >                    ++wordlength[1];
    > >            else if (nchar == 2)
    > >                    ++wordlength[2];
    > >            else if (nchar == 3)
    > >                    ++wordlength[3];
    > >            else if (nchar == 4)
    > >                    ++wordlength[4];
    > >            else if (nchar == 5)
    > >                    ++wordlength[5];
    > >            else if (nchar == 6)
    > >                    ++wordlength[6];

    >
    > As has been pointed out, you can write ++wordlength[nchar] (provided you
    > test that nchar is not too big).
    >
    > >            if (state == OUT)
    > >                    nchar = 0;
    > >    }
    > >    for (i = 50; i >= 0; --i)
    > >            if (wordlength[1] == verscale)
    > >                    for (i = verscale; i >= 0; --i)
    > >                            printf("%2d|  %c\n", verscale, x);
    > >    printf("  +------------------------------------\n");

    >
    > >    for (i = 0; i < 13; ++i)
    > >            printf("%3d", horscale);
    > >    printf("\n");

    >
    > This is all rather confused and I can't see what the objective is.  If
    > you say what you are trying to do here, it would help.
    >
    >
    >
    > >    return 0;
    > > }

    >
    > --
    > Ben.




    Ok guys I'm going to give it another go. You all have been extremely
    helpful and ill post an update when I'm done. It's amazing how
    powerful different ideas can be..............
     
    Ceriousmall, Feb 3, 2011
    #4
  5. Ceriousmall

    Chad Guest

    On Feb 2, 6:01 am, Ben Bacarisse <> wrote:
    > Ceriousmall <> writes:
    > > This is one of the programs from the K&R Ansi C Book that you have to
    > > write I've had a go at it but got kinda stumped here. So here's my
    > > effort so far...... And I'm aware there is a new ISO standard  for C
    > > C99 as of  March 2000 and I'm getting around to learning and adopting
    > > that as well but my  main concern is actually learning the
    > > language....

    >
    > > /* copyright 2011 Ceriousmall. . . .

    >
    > >    Program prints a histogram of the length of words in its input */

    >
    > > #include <stdio.h>

    >
    > > #define IN  1      /* inside a word */
    > > #define OUT 0      /* outside a word */

    >
    > > main()
    > > {
    > >    char x;
    > >    int c, i, n, nchar, state;
    > >         int wordlength[13];
    > >    int horscale[13], verscale[51];

    >
    > These "magic numbers" should be given names with a #define or an enum.
    >
    > >    x = '*';
    > >    state = OUT;

    >
    > When there are only two states I refer to avoid neutral names like
    > 'state' in favour of boolean variables (in C90 these are just int
    > variables) with a biased name: 'in_a_word' or 'between_words' and so
    > on.  That way, you don't need to name the states.
    >
    > >    n = nchar = 0;

    >
    > >    for (i = 0; i < 13; ++i) {
    > >            wordlength = 0;
    > >            horscale = i;
    > >    }
    > >    for (i = 0; i < 51; ++i)
    > >            verscale = i;

    >
    > You set verscale to i and never change it.  That's not a useful
    > array -- you can always just use the index instead; but then you also
    > don't use horscale in any useful way.  Without any idea what these
    > array are there for I can't suggest what you should be doing with them.
    > You don't need either for a simple histogram with horizontal bars.
    >
    > >    while ((c=getchar()) != EOF) {
    > >            if (c == ' ' || c == '\n' || c == '\t')
    > >                    state = OUT;
    > >            else if (state == OUT)   /* condition controlls the start */
    > >                    state = IN;      /* of a word */

    >
    > Look at the last if a bit more.  What happens when state is IN?  The
    > effect will be to leave state as IN so you could just have written
    >
    >    if (c == ' ' || c == '\n' || c == '\t')
    >         state = OUT;
    >    else state = IN;
    >


    I don't get what happens in the OP code if the state is IN.

    Chad
     
    Chad, Feb 4, 2011
    #5
  6. Chad <> writes:

    > On Feb 2, 6:01 am, Ben Bacarisse <> wrote:
    >> Ceriousmall <> writes:

    <snip>
    >> >            if (c == ' ' || c == '\n' || c == '\t')
    >> >                    state = OUT;
    >> >            else if (state == OUT)   /* condition controlls the start */
    >> >                    state = IN;      /* of a word */

    >>
    >> Look at the last if a bit more.  What happens when state is IN?  The
    >> effect will be to leave state as IN so you could just have written
    >>
    >>    if (c == ' ' || c == '\n' || c == '\t')
    >>         state = OUT;
    >>    else state = IN;

    >
    > I don't get what happens in the OP code if the state is IN.


    It might help if you said what the problem is -- the code does not seem
    very complex to me.

    When 'state' is 'IN', state will either be changed to 'OUT' (if 'c' is
    one of three particular characters) or it will be left alone and remain
    'IN'.

    --
    Ben.
     
    Ben Bacarisse, Feb 4, 2011
    #6
  7. Ceriousmall

    Ceriousmall Guest

    On Feb 3, 10:20 pm, Ben Bacarisse <> wrote:
    > Chad <> writes:
    > > On Feb 2, 6:01 am, Ben Bacarisse <> wrote:
    > >> Ceriousmall <> writes:

    > <snip>
    > >> >            if (c == ' ' || c == '\n' || c == '\t')
    > >> >                    state = OUT;
    > >> >            else if (state == OUT)   /* condition controlls the start */
    > >> >                    state = IN;      /* of a word */

    >
    > >> Look at the last if a bit more.  What happens when state is IN?  The
    > >> effect will be to leave state as IN so you could just have written

    >
    > >>    if (c == ' ' || c == '\n' || c == '\t')
    > >>         state = OUT;
    > >>    else state = IN;

    >
    > > I don't get what happens in the OP code if the state is IN.

    >
    > It might help if you said what the problem is -- the code does not seem
    > very complex to me.
    >
    > When 'state' is 'IN', state will either be changed to 'OUT' (if 'c' is
    > one of three particular characters) or it will be left alone and remain
    > 'IN'.
    >
    > --
    > Ben.


    =======================================================================================================================================
    this group has been extremely helpful to me so thanks for all the
    replies and after loads of effort this is my final product please
    comment and let me know what you guys think....

    /* copyright 2011 Ceriousmall. . . .

    Program prints a histogram of the length of words in its input */

    #include <stdio.h>

    #define ONE 1 /* set to the value of one */
    #define ZERO 0 /* set to nil value */
    #define LIMIT 14 /* horizontal scale & word size limits */
    #define VERLIMIT 51 /* verticle scale limit */

    main()
    {
    int i, x, in_word, mark_scale;
    long c, nchar;
    int mark[LIMIT];
    int horscale[LIMIT-ONE], verscale[VERLIMIT];
    long wordlength[LIMIT];

    nchar = mark_scale = ZERO;

    for (i = ZERO; i < LIMIT; ++i) {
    wordlength = ZERO;
    mark = ZERO;
    }
    for (i = ZERO; i < LIMIT-ONE; ++i)
    horscale = i;

    for (i = ZERO; i < VERLIMIT; ++i)
    verscale = i;

    while ((c=getchar()) != EOF) {
    if (c == ' ' || c == '\n' || c == '\t')
    in_word = ZERO;
    else
    in_word = ONE;

    if (in_word == ONE)
    ++nchar;
    else if (nchar < LIMIT) {
    for (i = ONE; i < LIMIT; ++i)
    if (nchar == i)
    ++wordlength;
    }
    else
    ++wordlength[LIMIT-ONE];

    if (in_word == ZERO)
    nchar = ZERO;
    }
    for (i = VERLIMIT-ONE; i > ZERO; --i) {
    for (x = ONE; x < LIMIT; ++x)
    if (wordlength[x] == verscale)
    ++mark_scale;

    if (mark_scale > ZERO)
    printf("%2d|", verscale);

    for (x = ONE; x < LIMIT; ++x)
    if (wordlength[x] == verscale) {
    printf(" %c", '*');
    ++mark[x];
    }
    else if (mark[x] == ZERO)
    printf(" %c", ' ');
    else
    printf(" %c", '*');
    printf("\n");
    }
    printf(" +---------------------------------------\n");

    for (i = ZERO; i < LIMIT-ONE; ++i)
    printf("%3d", horscale);

    printf(" >12\n");

    return 0;
    }
    _______________________________________________
    I'm really beginning to C the code now. . . . .
     
    Ceriousmall, Feb 6, 2011
    #7
  8. Ceriousmall

    Ceriousmall Guest

    I think this is better tho...................

    /* copyright 2011 Ceriousmall. . . .

    Program prints a histogram of the length of words in its input */

    #include <stdio.h>

    #define ONE 1 /* set to the value of one */
    #define ZERO 0 /* set to nil value */
    #define LIMIT 14 /* horizontal scale & word size limits */
    #define VERLIMIT 51 /* verticle scale limit */

    main()
    {
    int i, x, in_word, mark_scale;
    long c, nchar;
    int mark[LIMIT];
    int horscale[LIMIT-1], verscale[VERLIMIT];
    long wordlength[LIMIT];

    nchar = mark_scale = 0;

    for (i = 0; i < LIMIT; ++i) {
    wordlength = 0;
    mark = 0;
    }
    for (i = 0; i < LIMIT-1; ++i)
    horscale = i;

    for (i = 0; i < VERLIMIT; ++i)
    verscale = i;

    while ((c=getchar()) != EOF) {
    if (c == ' ' || c == '\n' || c == '\t')
    in_word = ZERO;
    else
    in_word = ONE;

    if (in_word == ONE)
    ++nchar;
    else if (nchar < LIMIT) {
    for (i = 1; i < LIMIT; ++i)
    if (nchar == i)
    ++wordlength;
    }
    else
    ++wordlength[LIMIT-1];

    if (in_word == ZERO)
    nchar = ZERO;
    }
    for (i = VERLIMIT-1; i > 0; --i) {
    for (x = 1; x < LIMIT; ++x)
    if (wordlength[x] == verscale)
    ++mark_scale;

    if (mark_scale > 0)
    printf("%2d|", verscale);

    for (x = 0; x < LIMIT; ++x)
    if (wordlength[x] == verscale) {
    printf(" %c", '*');
    ++mark[x];
    }
    else if (mark[x] == 0)
    printf(" %c", ' ');
    else
    printf(" %c", '*');
    printf("\n");
    }
    printf(" +---------------------------------------\n");

    for (i = 0; i < LIMIT-1; ++i)
    printf("%3d", horscale);

    printf(" >12\n");

    return 0;
    }
     
    Ceriousmall, Feb 6, 2011
    #8
  9. Ceriousmall <> writes:

    > I think this is better tho...................
    >
    > /* copyright 2011 Ceriousmall. . . .
    >
    > Program prints a histogram of the length of words in its input */
    >
    > #include <stdio.h>
    >
    > #define ONE 1 /* set to the value of one */
    > #define ZERO 0 /* set to nil value */


    I don't think these two comments really help.

    What's more, I don't think these macros are a good idea. By all means
    write something like

    #define TRUE 1
    #define FALSE 0

    or

    enum { false = 0, true = 1 };

    if you don't like writing 'in_word = 0;' but naming 0 'ZERO' and 1 'ONE'
    is not helpful. Consider the difference between

    #define LIMIT 14

    which is fine (though I might prefer a more descriptive name) and

    #define FOURTEEN 14

    > #define LIMIT 14 /* horizontal scale & word size limits */
    > #define VERLIMIT 51 /* verticle scale limit */
    >
    > main()


    This should be 'int main(void)'.

    > {
    > int i, x, in_word, mark_scale;
    > long c, nchar;


    Why are these two declared long? There's nothing wrong with that, it's
    just a bit odd and will puzzle anyone reading the code.

    > int mark[LIMIT];
    > int horscale[LIMIT-1], verscale[VERLIMIT];
    > long wordlength[LIMIT];
    >
    > nchar = mark_scale = 0;
    >
    > for (i = 0; i < LIMIT; ++i) {
    > wordlength = 0;
    > mark = 0;
    > }


    You can just initialise the arrays if you like:

    long wordlength[LIMIT] = {0};
    int mark[LIMIT] = {0};

    > for (i = 0; i < LIMIT-1; ++i)
    > horscale = i;
    >
    > for (i = 0; i < VERLIMIT; ++i)
    > verscale = i;


    These two are still not used. Yes, they are reference below but they
    serve no purpose.

    > while ((c=getchar()) != EOF) {
    > if (c == ' ' || c == '\n' || c == '\t')
    > in_word = ZERO;
    > else
    > in_word = ONE;


    I won't say what I'd write here -- you'd be appalled!

    > if (in_word == ONE)
    > ++nchar;
    > else if (nchar < LIMIT) {
    > for (i = 1; i < LIMIT; ++i)
    > if (nchar == i)
    > ++wordlength;


    Why have a loop here? You don't need to loop just to catch the case when
    nchar == i to increment wordlength. You can increment
    wordlength[nchar] directly (though the loop does catch the case when
    nchar is zero).

    > }
    > else
    > ++wordlength[LIMIT-1];
    >
    > if (in_word == ZERO)
    > nchar = ZERO;
    > }


    Look at the whole body of the while loop. You test for a space-like
    character and set 'in_word' then you immediately test it and do one
    thing if it is true (++nchar) and another thing if it is false
    (increment an element of wordlength and set nchar to zero).

    Can you see how you could write it much more simply?

    > for (i = VERLIMIT-1; i > 0; --i) {
    > for (x = 1; x < LIMIT; ++x)
    > if (wordlength[x] == verscale)
    > ++mark_scale;
    >
    > if (mark_scale > 0)
    > printf("%2d|", verscale);


    This confused me for a bit. Incrementing mark_scale and testing for > 0
    suggests and arithmetic purpose, but really it is a Boolean (true/false)
    value just like 'in_word' above.

    > for (x = 0; x < LIMIT; ++x)
    > if (wordlength[x] == verscale) {
    > printf(" %c", '*');
    > ++mark[x];
    > }
    > else if (mark[x] == 0)
    > printf(" %c", ' ');
    > else
    > printf(" %c", '*');
    > printf("\n");
    > }


    You can remove 'verscale' simply by writing 'i' instead of
    'verscale'. You can also get rid of the 'mark' array. In the inner
    loop, the condition that controls whether a star or a space is written
    is much simpler than you have made it. It took me a while to work out
    what the 'mark' array is being used for because it is really a Boolean
    (true/false) array yet you increment it's elements (just as with
    'mark_scale' above).

    You code simply ignores counts that are too big. That's not a good
    idea. The options would seem to be (a) scale the histogram so that
    highest bar is always VERLIMIT lines high; (b) just print a very tall
    histogram; or (c) limit the height but show that some bars are higher
    than actually shown.

    > printf(" +---------------------------------------\n");
    >
    > for (i = 0; i < LIMIT-1; ++i)
    > printf("%3d", horscale);


    horscale == i so there is no need for this array.

    There are also a few counting bugs. For example, input that has just
    one one-word line produces:

    1| *
    +---------------------------------------
    0 1 2 3 4 5 6 7 8 9 10 11 12 >12

    > printf(" >12\n");


    12? LIMIT is 14.

    > return 0;
    > }


    --
    Ben.
     
    Ben Bacarisse, Feb 6, 2011
    #9
  10. Ceriousmall

    Ceriousmall Guest

    On Feb 6, 7:05 pm, Ben Bacarisse <> wrote:

    Hey thanks for the pointers Ben I've certainly missed a lot. As simple
    as you make it seem, trust me it wasn't so for me but this is getting
    easier. These reviews are extremely useful to me as there isn't a
    whole lot of people I know that code in C...... Well I'm off to Vi
    again well Elvis that is............





    _______________________________________________
    Ceriously
    I'm really beginning to C the code now. . . . .
     
    Ceriousmall, Feb 7, 2011
    #10
  11. Ceriousmall

    Ceriousmall Guest

    I've had a go at it again and this is the result so feel free to
    comment.......
    I think this should be ok now and i can move on to the next chapter of
    the book....

    /* copyright 2011 Ceriousmall. . . .

    Program prints a histogram of the length of words in its input */

    #include <stdio.h>

    #define TRUE 1 /* state of mark or mark_scale */
    #define FALSE 0 /* state of mark or marker_sale */
    #define LIMIT 20 /* horizontal scale & word size limits */
    #define VERLIMIT 1000 /* verticle scale limit */

    main()
    {
    int c, nchar;
    int i, x, mark_scale;
    int mark[LIMIT+1];
    long wordlength[LIMIT+1];

    nchar = 0;
    mark_scale = FALSE;

    for (i = 0; i <= LIMIT; ++i) {
    wordlength = 0;
    mark = FALSE;
    }
    while ((c=getchar()) != EOF) {
    if (c == ' ' || c == '\n' || c == '\t') {
    if (nchar > 0 && nchar < LIMIT)
    ++wordlength[nchar];
    else if (nchar >= LIMIT)
    ++wordlength[LIMIT];
    nchar = 0;
    }
    else
    ++nchar;
    }
    for (i = VERLIMIT; i > 0; --i) {
    for (x = 1; x <= LIMIT; ++x)
    if (wordlength[x] == i)
    mark_scale = TRUE;

    if (mark_scale != FALSE)
    printf("%4d|", i);

    for (x = 1; x <= LIMIT; ++x)
    if (wordlength[x] == i) {
    printf("%3c", '*');
    mark[x] = TRUE;
    }
    else if (mark[x] == FALSE)
    printf("%3c", ' ');
    else
    printf("%3c", '*');
    printf("\n");
    }
    printf(" +---------------------------");
    printf("---------------------------------\n");
    printf(" ");

    for (i = 1; i <= LIMIT; ++i)
    printf("%3d", i);

    printf("++\n");

    return 0;
    }

    _____________________________________________)_
    Ceriously
    I'm really beginning to C the code now. . . . .
     
    Ceriousmall, Feb 7, 2011
    #11
  12. Ceriousmall

    Ceriousmall Guest

    I made only one change and that's in the way the horizontal scale is
    printed.......
    thus.....................
    printf(" +");
    for (i = 1; i <= LIMIT*3; ++i)
    printf("-");

    printf("\n ");
    for (i = 1; i <= LIMIT; ++i)
    printf("%3d", i);

    printf("++\n");

    return 0;
     
    Ceriousmall, Feb 7, 2011
    #12
  13. Ceriousmall

    Ceriousmall Guest

    On Feb 11, 4:51 pm, "io_x" <> wrote:
    > "Ceriousmall" <> ha scritto nel messaggionews:...
    > your code i did not understand much but
    > your program has a bug for me
    > if the input is somethig like
    > aaaa\n
    > hh^Z
    > where <ctrl-z>==^Z is the end of the input
    > it not find the 2 letter word hh
    >
    >  1|           *
    >   +------------------------------------------------------------
    >      1  2  3  4  5  6  7  8  9 10 11 12 13 14 15 16 17 18 19 20++
    >
    > instead this is my prog output with the same input:
    >   +-------------------- -------------------
    >  1:
    >  2:-
    >  3:
    >  4:-
    >  5:
    >  etc
    > there are 2 words (for the thefinition of word in text file)
    > one of 2 char and one of 4 chars




    Ok......... that's very interesting.........I'm forced to revisit that
    code................
     
    Ceriousmall, Feb 12, 2011
    #13
  14. Ceriousmall

    Ceriousmall Guest

    On Feb 11, 4:51 pm, "io_x" <> wrote:
    > "Ceriousmall" <> ha scritto nel messaggionews:...
    > your code i did not understand much but
    > your program has a bug for me
    > if the input is somethig like
    > aaaa\n
    > hh^Z
    > where <ctrl-z>==^Z is the end of the input
    > it not find the 2 letter word hh
    >
    >  1|           *
    >   +------------------------------------------------------------
    >      1  2  3  4  5  6  7  8  9 10 11 12 13 14 15 16 17 18 19 20++
    >
    > instead this is my prog output with the same input:
    >   +-------------------- -------------------
    >  1:
    >  2:-
    >  3:
    >  4:-
    >  5:
    >  etc
    > there are 2 words (for the thefinition of word in text file)
    > one of 2 char and one of 4 char

    ..
    ..
    ..
    Ok this is my revisited code, I believe this should solve the bug as
    stated above............

    /* Program prints a histogram of the length of words in its input */

    #include <stdio.h>

    #define TRUE 1
    #define FALSE 0
    #define LIMIT 20 /* horizontal scale & word size limits */
    #define VERLIMIT 1000 /* verticle scale limit */

    int main(void)
    {
    int c, i, x;
    int nchar, mark_scale, terminate;
    int mark[LIMIT+1];
    long wordlength[LIMIT+1];

    nchar = 0;
    mark_scale = FALSE;
    terminate = FALSE;

    for (i = 0; i <= LIMIT; ++i) {
    wordlength = 0;
    mark = FALSE;
    }
    while ((c = getchar()) && terminate == FALSE) {
    if (c >= '0' && c <= '9')
    printf("digit %d not valid for a word. . .\n", c-'0');
    else if (c == ' ' || c == '\n' || c == '\t') {
    if (nchar > 0 && nchar < LIMIT)
    ++wordlength[nchar];
    else if (nchar >= LIMIT)
    ++wordlength[LIMIT];
    nchar = 0;
    }
    else
    ++nchar;
    if (c == EOF)
    terminate = TRUE;
    }
    for (i = VERLIMIT; i >= 1; --i) {
    for (x = 1; x <= LIMIT; ++x)
    if (wordlength[x] == i)
    mark_scale = TRUE;

    if (mark_scale != FALSE)
    printf("%4d|", i);

    for (x = 1; x <= LIMIT; ++x)
    if (wordlength[x] == i) {
    printf("%3c", 'Ü');
    mark[x] = TRUE;
    }
    else if (mark[x] == FALSE)
    printf("%3c", ' ');
    else
    printf("%3c", 'Ü');
    printf("\n");
    }
    printf(" +");
    for (i = 1; i <= LIMIT*3; ++i)
    printf("-");

    printf("\n ");
    for (i = 1; i <= LIMIT; ++i)
    printf("%3d", i);

    printf("++\n");

    return 0;
    }


    _______________________________________________
    Ceriously
    I'm really beginning to C the code now. . . . .
     
    Ceriousmall, Feb 13, 2011
    #14
  15. Ceriousmall

    Ian Collins Guest

    On 02/13/11 07:35 PM, Ceriousmall wrote:
    > ..
    > Ok this is my revisited code, I believe this should solve the bug as
    > stated above............
    >
    > /* Program prints a histogram of the length of words in its input */
    >
    > #include<stdio.h>
    >
    > #define TRUE 1
    > #define FALSE 0
    > #define LIMIT 20 /* horizontal scale& word size limits */
    > #define VERLIMIT 1000 /* verticle scale limit */
    >
    > int main(void)
    > {
    > int c, i, x;
    > int nchar, mark_scale, terminate;
    > int mark[LIMIT+1];
    > long wordlength[LIMIT+1];
    >
    > nchar = 0;
    > mark_scale = FALSE;
    > terminate = FALSE;


    A lot of these are only used within a loop, declaring them at the start
    of the loop would make the code easier to read.

    > for (i = 0; i<= LIMIT; ++i) {
    > wordlength = 0;
    > mark = FALSE;
    > }
    > while ((c = getchar())&& terminate == FALSE) {
    > if (c>= '0'&& c<= '9')
    > printf("digit %d not valid for a word. . .\n", c-'0');


    Why? Try running the program with the source as the input!

    > else if (c == ' ' || c == '\n' || c == '\t') {


    Why not use isspace(c)?

    > if (nchar> 0&& nchar< LIMIT)
    > ++wordlength[nchar];
    > else if (nchar>= LIMIT)
    > ++wordlength[LIMIT];
    > nchar = 0;
    > }
    > else
    > ++nchar;
    > if (c == EOF)
    > terminate = TRUE;


    You should test for EOF as early as possible, in the while condition
    would be best.

    > }
    > for (i = VERLIMIT; i>= 1; --i) {


    Given a large value for VERLIMIT, this will print a lot of blank lines.
    it might be better to track the maximum count in a bucket and use that
    here.

    --
    Ian Collins
     
    Ian Collins, Feb 13, 2011
    #15
  16. Ceriousmall

    Ike Naar Guest

    On 2011-02-13, Ian Collins <> wrote:
    > On 02/13/11 07:35 PM, Ceriousmall wrote:
    >> else if (c == ' ' || c == '\n' || c == '\t') {

    >
    > Why not use isspace(c)?


    It's not the same.
    isspace() also includes carriage return, form feed and vertical tab.
     
    Ike Naar, Feb 13, 2011
    #16
  17. Ceriousmall

    Ian Collins Guest

    On 02/13/11 10:34 PM, Ike Naar wrote:
    > On 2011-02-13, Ian Collins<> wrote:
    >> On 02/13/11 07:35 PM, Ceriousmall wrote:
    >>> else if (c == ' ' || c == '\n' || c == '\t') {

    >>
    >> Why not use isspace(c)?

    >
    > It's not the same.
    > isspace() also includes carriage return, form feed and vertical tab.


    All of which separate words.

    --
    Ian Collins
     
    Ian Collins, Feb 13, 2011
    #17
  18. Ian Collins <> writes:

    > On 02/13/11 07:35 PM, Ceriousmall wrote:

    <snip>
    >> else if (c == ' ' || c == '\n' || c == '\t') {

    >
    > Why not use isspace(c)?


    Ceriousmall seems to be on chapter 1. The isxxx macros are not
    introduced until later. Indeed, K&R use c == ' ' || c == '\n' || c ==
    '\t' in a word counting example in chapter 1 so it is the obvious choice
    at this stage.

    <snip>
    --
    Ben.
     
    Ben Bacarisse, Feb 13, 2011
    #18
  19. Ceriousmall

    Ceriousmall Guest

    On Feb 13, 10:45 am, Ben Bacarisse <> wrote:
    > Ian Collins <> writes:
    > > On 02/13/11 07:35 PM, Ceriousmall wrote:

    > <snip>
    > >>                else if (c == ' ' || c == '\n' || c == '\t') {

    >
    > > Why not use isspace(c)?

    >
    > Ceriousmall seems to be on chapter 1.  The isxxx macros are not
    > introduced until later.  Indeed, K&R use c == ' ' || c == '\n' || c ==
    > '\t' in a word counting example in chapter 1 so it is the obvious choice
    > at this stage.
    >
    > <snip>
    > --
    > Ben.


    ..
    ..
    ..
    Thank U Ben indeed.... page 28 Character arrays to be
    precise............... this forum is my link too other C programmers
    and all the help is greatly appreciated....
     
    Ceriousmall, Feb 13, 2011
    #19
  20. Ceriousmall <> writes:

    > On Feb 11, 4:51 pm, "io_x" <> wrote:
    >> "Ceriousmall" <> ha scritto nel messaggionews:...
    >> your program has a bug for me
    >> if the input is somethig like
    >> aaaa\n
    >> hh^Z
    >> where <ctrl-z>==^Z is the end of the input
    >> it not find the 2 letter word hh

    <snip>
    > Ok this is my revisited code, I believe this should solve the bug as
    > stated above............


    The end result is worse, I think.

    <snip>
    > while ((c = getchar()) && terminate == FALSE) {
    > if (c >= '0' && c <= '9')
    > printf("digit %d not valid for a word. . .\n", c-'0');
    > else if (c == ' ' || c == '\n' || c == '\t') {
    > if (nchar > 0 && nchar < LIMIT)
    > ++wordlength[nchar];
    > else if (nchar >= LIMIT)
    > ++wordlength[LIMIT];
    > nchar = 0;
    > }
    > else
    > ++nchar;
    > if (c == EOF)
    > terminate = TRUE;


    First, using an assignment in a larger expression but where the value is
    to be ignored is very peculiar. It's likely to confuse people. What is
    more, a null byt will terminate the loop which might surprise the user
    even more.

    Secondly, putting the ch = getchar() test *before* the other half of the
    && means that, on some systems, EOF no longer works. After detecting
    EOF you try to read again. It's best to do any more input once you've
    seen EOF and you can do that by switching the && order:

    terminate == FALSE && (c = getchar())

    However you should really put the c = getchar() at the start of the loop
    body.

    If you do that, you'll see that ch == EOF exactly mirrors the value of
    'terminate' so there is no need for that exatr variable. Instead you
    could write:

    ch = 0; /* anything but EOF */
    while (ch != EOF) {
    ch = getchar();
    /* The rest of your body but without the last 'if' */
    }

    But, finally, C has a loop for exactly this kind of pattern: do
    .... while. So I'd write:

    do {
    ch = getchar();
    /* The rest of your body but without the last 'if' */
    } while (ch != EOF) {

    The end result is simpler.

    Rather then just say "use a do loop" I've tried to show you can be
    thinking about your programs be showing you a sequences of small
    improvements.

    <snip>
    --
    Ben.
     
    Ben Bacarisse, Feb 13, 2011
    #20
    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. Peter Strøiman
    Replies:
    1
    Views:
    2,112
    Peter Strøiman
    Aug 23, 2005
  2. Richard Heathfield
    Replies:
    7
    Views:
    381
    Barry Schwarz
    Oct 5, 2003
  3. utab

    Words Words

    utab, Feb 16, 2006, in forum: C++
    Replies:
    6
    Views:
    437
    Daniel T.
    Feb 16, 2006
  4. trapeze

    C++ : need quick tips for easy histogram

    trapeze, Sep 7, 2007, in forum: C Programming
    Replies:
    1
    Views:
    422
    trapeze
    Sep 7, 2007
  5. BerlinBrown
    Replies:
    6
    Views:
    4,664
Loading...

Share This Page