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

[Ceriousmall]

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;
}

 

[Malcolm McLean]

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

 

[Ben Bacarisse]

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.

 

[Ceriousmall]

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;
}

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

 

[Chad]

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

 

[Ben Bacarisse]

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

 

[Ceriousmall]

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

=======================================================================================================================================
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]

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;
}

 

[Ben Bacarisse]

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.

 

[Ceriousmall]

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]

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]

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]

"Ceriousmall" <> ha scritto nel messaggioyour 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]

"Ceriousmall" <> ha scritto nel messaggioyour 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. . . . .

 

[Ian Collins]

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

 

[Ike Naar]

Why not use isspace(c)?

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

 

[Ian Collins]

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

All of which separate words.

 

[Ben Bacarisse]

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

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>

 

[Ceriousmall]

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>

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

 

[Ben Bacarisse]

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

The end result is worse, I think.

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>