Arithmetic error

[Andy Foot]

Hello

I have recently started a distance-learning C course.

I am writing a program to input some numbers and calculate they're mean
(average). However there is a bug: if the numbers are 1,2,3 then the
calculation is correct (mean=2) however if the numbers are 1,2 then the
answer is wrong (mean=1, should be 1.5).

What's going on!

Thanks,
Andy

#include <stdio.h>
void main()
{
char buf[BUFSIZ];
int a=0,b=0;
while(strcmp(gets(buf),"")!=0){
a++; b+=atoi((char*)buf);
}
printf("mean=%f",(float)(b/a));
}

 

[Ian Collins]

Hello

I have recently started a distance-learning C course.

I am writing a program to input some numbers and calculate they're mean
(average). However there is a bug: if the numbers are 1,2,3 then the
calculation is correct (mean=2) however if the numbers are 1,2 then the
answer is wrong (mean=1, should be 1.5).

What's going on!

#include<stdio.h>
void main()
{
char buf[BUFSIZ];
int a=0,b=0;
while(strcmp(gets(buf),"")!=0){
a++; b+=atoi((char*)buf);

atoi should be avoided (it can't indicate an error), have a look at
strtol instead.

}
printf("mean=%f",(float)(b/a));

You are casting the result of (int)3/(int)2, which is 1. Make b a
double and call it something meaningful like sum.

 

[Nick Keighley]

I have recently started a distance-learning C course.

I am writing a program to input some numbers and calculate [their] mean
(average). However there is a bug: if the numbers are 1,2,3 then the
calculation is correct (mean=2) however if the numbers are 1,2 then the
answer is wrong (mean=1, should be 1.5).

What's going on!

you're doing integer maths when you mean to do floating point maths

6 / 3 equals 2
but 3 / 2 equals 1

#include <stdio.h>
void main()

the correct signature for main is
int main (void)
( or int main (int argc, char *argv[]) )

main always returns int

{
    char buf[BUFSIZ];
    int a=0,b=0;

these aren't very clear names. How about "count" and "sum"?

    while(strcmp(gets(buf),"")!=0){

gets() should never be used. What if the user types more than BUFSIZ
characters?

        a++; b+=atoi((char*)buf);

atoi() has poor error handling. Use one of the alternatives (strtoul
etc.). What is the point of the cast to char*? In general avoid casts
unless you *really* need them.

    }
    printf("mean=%f",(float)(b/a));

and this is where things go wrong

printf ("mean=%f\n", (double)b / a);

You need the \n or the output may not appear on some systems.
Prefer double to float. Most of the C library uses double so it is
rare float buys you anything.
It is better if you use more whitespace in your layout. It makes it
easier to read.

Oh and since main *always* returns an int you need
return 0;

}

Happy programming, Nick

 

[BartC]

Hello

I have recently started a distance-learning C course.

I am writing a program to input some numbers and calculate they're mean
(average). However there is a bug: if the numbers are 1,2,3 then the
calculation is correct (mean=2) however if the numbers are 1,2 then the
answer is wrong (mean=1, should be 1.5).

What's going on!

Try this version. The main change is moving the (float):

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int main (void)
{
char buf[BUFSIZ];
int a=0,b=0;
puts("Enter one number per line. Blank line to end:");
while(strcmp(gets(buf),"")!=0){
a++; b+=atoi((char*)buf);
}
if (a) printf("Mean=%f\n",((float)b/a));
}

BTW you should use a newer textbook. (And where is BUFSIZ declared? Seems to
compile though..)

 

[Ben Bacarisse]

I have recently started a distance-learning C course.

I won't re-answer your question, but there are a number of things about
your code that make me a little worried about this course. Of course,
it may be that you've gone on ahead and some of these issues come from
an admirable sense of exploration. In which case, just make sure that
when you explore you do so with some good documentation.

#include <stdio.h>
void main()

main returns int. It's also a good habit to write (void) rather than ()
when a function takes no arguments. It make no difference here, but
there is no reason to do it right from the very start. The reason it
matters in bigger programs is that the f() form is old syntax that has a
special, rather lax, meaning. The f(void) form instructs the compiler
to tell you when you call f with arguments and you always want as much
help as possible from the compiler.

{
char buf[BUFSIZ];

There's no particular reason to use BUFSIZ here. Someone who does not
know what it is will wonder if it's big enough (or too big) and someone
who does know will wonder why that size is important to your program.
I'd pick an explicit size myself.

int a=0,b=0;

Those are not really very helpful names. I know it seems daft, in a
half dozen line program, to worry about names, but why not start out by
thinking up good names? They don't have to be long: "count" and "sum"
would be OK, I think.

while(strcmp(gets(buf),"")!=0){

There are several issues here. One is that gets is a really bad
function -- so bad it has suffered the ultimate fate of being removed
form the next C standard and it is officially deprecated in the current
one. The second issue is that gets returns a null pointer when the end
of the input is reached. Passing a null pointer as one of strcmp's
arguments is a bad thing -- anything can happen.

The possibility of the input running out may not be something that you
care about right now since you are typing at your program but, again,
it's good to avoid getting into bad habits. Whilst I'd not use gets,
you could fix this particular issue like this:

while (gets(buf) != NULL && strcmp(buf, "") != 0) { ...

The third issue is just whether you are testing the right thing. Your
test stop the loop when an empty line is typed but does not stop when
there is no sane input (for example "xyz" is not a number but you go
ahead and try to use it as one).

I'd use scanf for this. scanf has a bad reputation amongst C experts
but it is actually very good for this sort of program. It's bad rap
comes from the fact that you can't easily control the input format, but
that is not a worry to you here. It has the lovely property of telling
you when things have worked out -- if you scan for one int, it returns 1
when an it was found so can simply write:

while (scanf("%d", &b) == 1) {...

Any errors and the loop stops. Of course that may not be what was
wanted and you may not have been told about scanf yet (and it's rather
odd &b argument) but using gets and atoi does not seem like a good thing
to learn to do.

If scanf is not an option, use fgets instead of gets. It gets told how
big the buffer is so it can't go wrong in the same horrible ways that
gets can.

a++; b+=atoi((char*)buf);

The cast is not needed. It has no effect.

}
printf("mean=%f",(float)(b/a));

You should have a \n after the %f. Again it's a matter of good habits.
Your system may well be happy with what you have but failing to end your
output with a newline can mess up on some systems.

}

It's better to end main with "return 0;". The latest C standard has a
special exemption for the main function but, again, it's a good habit to
get into -- if a function has a value, you must remember to return one.
I know that in this case, you didn't because you though main was a void
function so this really the first just my point all over again.

This is, I know, a lot of comments on a small program. Please don't be
discouraged. Think of it like a brain dump -- not all of them are
important but I find it easier just to say everything that comes into my
head.

 

[Ike Naar]

(And where is BUFSIZ declared? Seems to compile though..)

<stdio.h>:
The macros are [...] BUFSIZ which expands to an integer constant
expression that is the size of the buffer used by the setbuf
function;

 

[Ike Naar]

(And where is BUFSIZ declared? Seems to compile though..)

<stdio.h>:
The macros are [...] BUFSIZ which expands to an integer constant
expression that is the size of the buffer used by the setbuf
function;

 

[Fred]

I have recently started a distance-learning C course.

I won't re-answer your question, but there are a number of things about
your code that make me a little worried about this course.  Of course,
it may be that you've gone on ahead and some of these issues come from
an admirable sense of exploration.  In which case, just make sure that
when you explore you do so with some good documentation.

#include <stdio.h>
void main()

main returns int.  It's also a good habit to write (void) rather than ()
when a function takes no arguments.  It make no difference here, but
there is no reason to do it right from the very start.  The reason it
matters in bigger programs is that the f() form is old syntax that has a
special, rather lax, meaning.  The f(void) form instructs the compiler
to tell you when you call f with arguments and you always want as much
help as possible from the compiler.

{
    char buf[BUFSIZ];

There's no particular reason to use BUFSIZ here.  Someone who does not
know what it is will wonder if it's big enough (or too big) and someone
who does know will wonder why that size is important to your program.
I'd pick an explicit size myself.

    int a=0,b=0;

Those are not really very helpful names.  I know it seems daft, in a
half dozen line program, to worry about names, but why not start out by
thinking up good names?  They don't have to be long: "count" and "sum"
would be OK, I think.

    while(strcmp(gets(buf),"")!=0){

There are several issues here.  One is that gets is a really bad
function -- so bad it has suffered the ultimate fate of being removed
form the next C standard and it is officially deprecated in the current
one.  The second issue is that gets returns a null pointer when the end
of the input is reached.  Passing a null pointer as one of strcmp's
arguments is a bad thing -- anything can happen.

The possibility of the input running out may not be something that you
care about right now since you are typing at your program but, again,
it's good to avoid getting into bad habits.  Whilst I'd not use gets,
you could fix this particular issue like this:

  while (gets(buf) != NULL && strcmp(buf, "") != 0) { ...

The third issue is just whether you are testing the right thing.  Your
test stop the loop when an empty line is typed but does not stop when
there is no sane input (for example "xyz" is not a number but you go
ahead and try to use it as one).

I'd use scanf for this.  scanf has a bad reputation amongst C experts
but it is actually very good for this sort of program.  It's bad rap
comes from the fact that you can't easily control the input format, but
that is not a worry to you here.  It has the lovely property of telling
you when things have worked out -- if you scan for one int, it returns 1
when an it was found so can simply write:

  while (scanf("%d", &b) == 1) {...

Any errors and the loop stops.  Of course that may not be what was
wanted and you may not have been told about scanf yet (and it's rather
odd &b argument) but using gets and atoi does not seem like a good thing
to learn to do.

If scanf is not an option, use fgets instead of gets.  It gets told how
big the buffer is so it can't go wrong in the same horrible ways that
gets can.

        a++; b+=atoi((char*)buf);

The cast is not needed.  It has no effect.

    }
    printf("mean=%f",(float)(b/a));

You should have a \n after the %f.  Again it's a matter of good habits.
Your system may well be happy with what you have but failing to end your
output with a newline can mess up on some systems.

}

It's better to end main with "return 0;".  The latest C standard has a
special exemption for the main function but, again, it's a good habit to
get into -- if a function has a value, you must remember to return one.
I know that in this case, you didn't because you though main was a void
function so this really the first just my point all over again.

This is, I know, a lot of comments on a small program.  Please don't be
discouraged.  Think of it like a brain dump -- not all of them are
important but I find it easier just to say everything that comes into my
head.

This code can still fail.
Try to fond the mean of these three numbers:
MAX_INT - 1
MAX_INT - 2
MAX_INT - 3

The mean is (MAX_INT-2), but the algorithm used will break due to
overflow.