Code Review: Averaging program

[Mark A. Nicolosi]

I've been trying to learn C for quite a while. But I've had trouble
with the
lack of good quality online text (some of it's alright). But I finally
bought
a book on C, Practical C. I like it alot. I'm pretty familiar with the
C basics,
because I've been trying to learn it for a while. But anyone, I wrote
a program
to average any number of arguments given at the command line. I
thought it might
be useful to get feedback about my coding style or anything I've done
wrong. It
compile just fine with gcc (with -Wall -ansi -pedantic). Anyways, here
it is:

/* average.c - Average several numbers together. */
/* Copyright (C) 2003 Mark A. Nicolosi */

#include <stdio.h>
#include <malloc.h>

float average (int *numbers, int how_many);

int main (int argc, char **argv)
{
int how_many; /* Number of numbers to average. */
int i; /* Index for for loop. */
int *numbers; /* Array of numbers to be averaged. */
float answer; /* Variable to hold the answer. */

how_many = argc - 1; /* Every argument after argv[0] should be
number to average */
if (how_many == 0) /* If TRUE then not called with any arguments.
*/
{
printf ("Usage: %s <num1> <num2> ...\n", argv[0]);
return 1;
}

/* Allocate enough memory to hold the array of numbers to be
averaged. */
numbers = malloc (sizeof (int) * how_many);


/* Transfer the string arrays in argv[i + 1] to an array of
integars. */
for (i = 0; i < how_many; i++)
{
sscanf (argv[i + 1], "%d", &numbers[i]);
}

/* Now that we've got an array of integars to average, average 'em.
*/
answer = average (numbers, how_many);
printf ("%g\n", answer);

/* Free the array of numbers. */

free (numbers);

return 0;
}

float average (int *numbers, int how_many)
{
float answer = 0.0; /* Variable to hold the answer. */
int i; /* Index for for loop. */

/* Loop to add all the numbers in the number[] array. */
for (i = 0; i < how_many; i++)
{
answer = answer + numbers[i];
}

/* Now divide by the number of numbers. */
answer = answer / how_many;

return answer;
}

I copy and pasted that in to links, so I'm not sure if it'll look
alight :)
Thanks.

--
Mark A. Nicolosi

[Kevin Easton]

Re: Code Review: Averaging program

Mark A. Nicolosi <markan@foobox. net> wrote:
[...][color=blue]
> /* average.c - Average several numbers together. */
> /* Copyright (C) 2003 Mark A. Nicolosi */
>
> #include <stdio.h>
> #include <malloc.h>[/color]

malloc.h is a non-standard header - to get a declaration of malloc, you
only need to #include <stdlib.h>.
[color=blue]
> float average (int *numbers, int how_many);[/color]

In general, you should choose "double" for floating point numbers,
unless you have a specific reason for choosing "float".
[color=blue]
> int main (int argc, char **argv)
> {
> int how_many; /* Number of numbers to average. */
> int i; /* Index for for loop. */
> int *numbers; /* Array of numbers to be averaged. */
> float answer; /* Variable to hold the answer. */
>
> how_many = argc - 1; /* Every argument after argv[0] should be
> number to average */
> if (how_many == 0) /* If TRUE then not called with any arguments.
> */[/color]

It is possible for argc to be zero, in which case this if branch will
not be taken.
[color=blue]
> {
> printf ("Usage: %s <num1> <num2> ...\n", argv[0]);[/color]

....which is a good thing, because in that case argv[0] would be NULL and
you would have a problem here.
[color=blue]
> return 1;
> }
>
> /* Allocate enough memory to hold the array of numbers to be
> averaged. */
> numbers = malloc (sizeof (int) * how_many);[/color]

You should check for malloc() returning NULL here, and report an error.
As your program is currently written, if malloc fails, it will still try
to access memory through the pointer "numbers".
[color=blue]
> /* Transfer the string arrays in argv[i + 1] to an array of
> integars. */
> for (i = 0; i < how_many; i++)
> {
> sscanf (argv[i + 1], "%d", &numbers[i]);
> }[/color]

You should check that the return value of sscanf - it tells you if it
successfully performed the conversion. If it didn't, the value in
numbers[i] will be garbage, leading to a garbage result (formally, the
behaviour of the program in this instance is undefined, because it uses
an indeterminate value in a calculation).

Alternatively you can pre-fill the numbers array with 0.0 values.
[color=blue]
> /* Now that we've got an array of integars to average, average 'em.
> */
> answer = average (numbers, how_many);
> printf ("%g\n", answer);
>
> /* Free the array of numbers. */
>
> free (numbers);
>
> return 0;
> }
>
> float average (int *numbers, int how_many)
> {
> float answer = 0.0; /* Variable to hold the answer. */
> int i; /* Index for for loop. */
>
> /* Loop to add all the numbers in the number[] array. */
> for (i = 0; i < how_many; i++)
> {
> answer = answer + numbers[i];
> }
>
> /* Now divide by the number of numbers. */
> answer = answer / how_many;
>
> return answer;
> }[/color]

You can rewrite this program so that it doesn't need any dynamic memory
allocation at all, because you don't need to store every single number -
you can just keep a running total. Here is an alternative version of
your program, that uses the fact that the 'argv' array is terminated
with a NULL pointer:

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

int main (int argc, char **argv)
{
double total = 0.0;
int how_many = 0;
int n;

while (*++argv) {
if (sscanf(*argv, "%d", &n) == 1) {
total += n;
how_many++;
}
}

if (how_many > 0) {
printf("%g\n", total / how_many);
return 0;
}

if (argv[0]) {
printf ("Usage: %s <num1> <num2> ...\n", argv[0]);
}

return 1;
}

[Isaac Mushinsky]

Re: Code Review: Averaging program

Mark A. Nicolosi wrote:
[color=blue]
> I've been trying to learn C for quite a while. But I've had trouble
> with the
> lack of good quality online text (some of it's alright). But I finally
> bought
> a book on C, Practical C. I like it alot. I'm pretty familiar with the
> C basics,
> because I've been trying to learn it for a while. But anyone, I wrote
> a program
> to average any number of arguments given at the command line. I
> thought it might
> be useful to get feedback about my coding style or anything I've done
> wrong. It
> compile just fine with gcc (with -Wall -ansi -pedantic). Anyways, here
> it is:
>
> /* average.c - Average several numbers together. */
> /* Copyright (C) 2003 Mark A. Nicolosi */
>
> #include <stdio.h>
> #include <malloc.h>
>
> float average (int *numbers, int how_many);
>
> int main (int argc, char **argv)
> {
> int how_many; /* Number of numbers to average. */
> int i; /* Index for for loop. */
> int *numbers; /* Array of numbers to be averaged. */
> float answer; /* Variable to hold the answer. */
>
> how_many = argc - 1; /* Every argument after argv[0] should be
> number to average */
> if (how_many == 0) /* If TRUE then not called with any arguments.
> */
> {
> printf ("Usage: %s <num1> <num2> ...\n", argv[0]);
> return 1;
> }
>
> /* Allocate enough memory to hold the array of numbers to be
> averaged. */
> numbers = malloc (sizeof (int) * how_many);
>
>
> /* Transfer the string arrays in argv[i + 1] to an array of
> integars. */
> for (i = 0; i < how_many; i++)
> {
> sscanf (argv[i + 1], "%d", &numbers[i]);
> }
>
> /* Now that we've got an array of integars to average, average 'em.
> */
> answer = average (numbers, how_many);
> printf ("%g\n", answer);
>
> /* Free the array of numbers. */
>
> free (numbers);
>
> return 0;
> }
>
> float average (int *numbers, int how_many)
> {
> float answer = 0.0; /* Variable to hold the answer. */
> int i; /* Index for for loop. */
>
> /* Loop to add all the numbers in the number[] array. */
> for (i = 0; i < how_many; i++)
> {
> answer = answer + numbers[i];
> }
>
> /* Now divide by the number of numbers. */
> answer = answer / how_many;
>
> return answer;
> }
>
> I copy and pasted that in to links, so I'm not sure if it'll look
> alight :)
> Thanks.
>[/color]

It will work, but
1. better use atoi() than sscanf()
2. There are a lot of unnecessary manipulations, you really do not need to
allocate another array to compute the average. e.g.

#include <stdlib.h>
#include <stdio.h>
int main (int argc, char **argv)
{
int i, n;
float answer=0;
for (i=0; i < argc; i++)
answer = (answer*i+atoi( argv[i]))/(i+1);
printf (... answer ...);
return 0;
}

[Kevin Easton]

Re: Code Review: Averaging program

I wrote:
[...][color=blue]
> #include <stdio.h>
> #include <stdlib.h>
>
> int main (int argc, char **argv)
> {
> double total = 0.0;
> int how_many = 0;
> int n;
>
> while (*++argv) {[/color]

Of course I made a classic blunder here - assuming that argc is greater
than 0. And I modify argv and then try to get the original argv[0]
later.

That should be

char *program;

if (argc < 2) {
if (argc < 1) {
program = "average";
} else {
program = argv[0];
}

printf ("Usage: %s <num1> <num2> ...\n", program);
return 1;
}

while (*++argv) {
/* ... */

Then remove the similar printf from the bottom.

- Kevin.

[Isaac Mushinsky]

Re: Code Review: Averaging program

Rather ugly.
It will work, but
1. better use atoi() than sscanf()
2. There are a lot of unnecessary manipulations, you really do not need to
allocate another array to compute the average, e.g.

#include <stdlib.h>
#include <stdio.h>
int main (int argc, char **argv)
{
int i;
double answer=0;

#include <stdlib.h>
#include <stdio.h>
int main (int argc, char **argv)
{
int i;
double answer=0;

for (i=1; i < argc ; i++)
answer = (answer*(i-1)+atoi(argv[i]))/i;
printf ("answer %f\n", answer);
return 0;
}

/* sorry I had to repost; in first message the cycle started at 0
incorrectly */

[Isaac Mushinsky]

Re: Code Review: Averaging program

Mark A. Nicolosi wrote:
[color=blue]
> I've been trying to learn C for quite a while. But I've had trouble
> with the
> lack of good quality online text (some of it's alright). But I finally
> bought
> a book on C, Practical C. I like it alot. I'm pretty familiar with the
> C basics,
> because I've been trying to learn it for a while. But anyone, I wrote
> a program
> to average any number of arguments given at the command line. I
> thought it might
> be useful to get feedback about my coding style or anything I've done
> wrong. It
> compile just fine with gcc (with -Wall -ansi -pedantic). Anyways, here
> it is:
>
> /* average.c - Average several numbers together. */
> /* Copyright (C) 2003 Mark A. Nicolosi */
>
> #include <stdio.h>
> #include <malloc.h>
>
> float average (int *numbers, int how_many);
>
> int main (int argc, char **argv)
> {
> int how_many; /* Number of numbers to average. */
> int i; /* Index for for loop. */
> int *numbers; /* Array of numbers to be averaged. */
> float answer; /* Variable to hold the answer. */
>
> how_many = argc - 1; /* Every argument after argv[0] should be
> number to average */
> if (how_many == 0) /* If TRUE then not called with any arguments.
> */
> {
> printf ("Usage: %s <num1> <num2> ...\n", argv[0]);
> return 1;
> }
>
> /* Allocate enough memory to hold the array of numbers to be
> averaged. */
> numbers = malloc (sizeof (int) * how_many);
>
>
> /* Transfer the string arrays in argv[i + 1] to an array of
> integars. */
> for (i = 0; i < how_many; i++)
> {
> sscanf (argv[i + 1], "%d", &numbers[i]);
> }
>
> /* Now that we've got an array of integars to average, average 'em.
> */
> answer = average (numbers, how_many);
> printf ("%g\n", answer);
>
> /* Free the array of numbers. */
>
> free (numbers);
>
> return 0;
> }
>
> float average (int *numbers, int how_many)
> {
> float answer = 0.0; /* Variable to hold the answer. */
> int i; /* Index for for loop. */
>
> /* Loop to add all the numbers in the number[] array. */
> for (i = 0; i < how_many; i++)
> {
> answer = answer + numbers[i];
> }
>
> /* Now divide by the number of numbers. */
> answer = answer / how_many;
>
> return answer;
> }
>
> I copy and pasted that in to links, so I'm not sure if it'll look
> alight :)
> Thanks.
>[/color]

It will work, but
1. better use atoi() than sscanf()
2. There are a lot of unnecessary manipulations, you really do not need to
allocate another array to compute the average. e.g.

#include <stdlib.h>
#include <stdio.h>
int main (int argc, char **argv)
{
int i;
float answer=0;
for (i=0; i < argc; i++)
answer = (answer*i+atoi( argv[i]))/(i+1);
printf (... answer ...);
return 0;
}

[Isaac Mushinsky]

Re: Code Review: Averaging program

Mark A. Nicolosi wrote:
[color=blue]
> I've been trying to learn C for quite a while. But I've had trouble
> with the
> lack of good quality online text (some of it's alright). But I finally
> bought
> a book on C, Practical C. I like it alot. I'm pretty familiar with the
> C basics,
> because I've been trying to learn it for a while. But anyone, I wrote
> a program
> to average any number of arguments given at the command line. I
> thought it might
> be useful to get feedback about my coding style or anything I've done
> wrong. It
> compile just fine with gcc (with -Wall -ansi -pedantic). Anyways, here
> it is:
>
> /* average.c - Average several numbers together. */
> /* Copyright (C) 2003 Mark A. Nicolosi */
>
> #include <stdio.h>
> #include <malloc.h>
>
> float average (int *numbers, int how_many);
>
> int main (int argc, char **argv)
> {
> int how_many; /* Number of numbers to average. */
> int i; /* Index for for loop. */
> int *numbers; /* Array of numbers to be averaged. */
> float answer; /* Variable to hold the answer. */
>
> how_many = argc - 1; /* Every argument after argv[0] should be
> number to average */
> if (how_many == 0) /* If TRUE then not called with any arguments.
> */
> {
> printf ("Usage: %s <num1> <num2> ...\n", argv[0]);
> return 1;
> }
>
> /* Allocate enough memory to hold the array of numbers to be
> averaged. */
> numbers = malloc (sizeof (int) * how_many);
>
>
> /* Transfer the string arrays in argv[i + 1] to an array of
> integars. */
> for (i = 0; i < how_many; i++)
> {
> sscanf (argv[i + 1], "%d", &numbers[i]);
> }
>
> /* Now that we've got an array of integars to average, average 'em.
> */
> answer = average (numbers, how_many);
> printf ("%g\n", answer);
>
> /* Free the array of numbers. */
>
> free (numbers);
>
> return 0;
> }
>
> float average (int *numbers, int how_many)
> {
> float answer = 0.0; /* Variable to hold the answer. */
> int i; /* Index for for loop. */
>
> /* Loop to add all the numbers in the number[] array. */
> for (i = 0; i < how_many; i++)
> {
> answer = answer + numbers[i];
> }
>
> /* Now divide by the number of numbers. */
> answer = answer / how_many;
>
> return answer;
> }
>
> I copy and pasted that in to links, so I'm not sure if it'll look
> alight :)
> Thanks.
>[/color]

It will work, but
1. better use atoi() than sscanf()
2. There are a lot of unnecessary manipulations, you really do not need to
allocate another array to compute the average. e.g.

#include <stdlib.h>
#include <stdio.h>
int main (int argc, char **argv)
{
int i, n;
float answer=0;
for (i=0; i < argc; i++)
answer = (answer*i+atoi( argv[i]))/(i+1);
printf (... answer ...);
return 0;
}

[Kevin Easton]

Re: Code Review: Averaging program

I wrote:
[...][color=blue]
> #include <stdio.h>
> #include <stdlib.h>
>
> int main (int argc, char **argv)
> {
> double total = 0.0;
> int how_many = 0;
> int n;
>
> while (*++argv) {[/color]

Of course I made a classic blunder here - assuming that argc is greater
than 0. And I modify argv and then try to get the original argv[0]
later.

That should be

char *program;

if (argc < 2) {
if (argc < 1) {
program = "average";
} else {
program = argv[0];
}

printf ("Usage: %s <num1> <num2> ...\n", program);
return 1;
}

while (*++argv) {
/* ... */

Then remove the similar printf from the bottom.

- Kevin.

[Isaac Mushinsky]

Re: Code Review: Averaging program

Rather ugly.
It will work, but
1. better use atoi() than sscanf()
2. There are a lot of unnecessary manipulations, you really do not need to
allocate another array to compute the average, e.g.

#include <stdlib.h>
#include <stdio.h>
int main (int argc, char **argv)
{
int i;
double answer=0;

#include <stdlib.h>
#include <stdio.h>
int main (int argc, char **argv)
{
int i;
double answer=0;

for (i=1; i < argc ; i++)
answer = (answer*(i-1)+atoi(argv[i]))/i;
printf ("answer %f\n", answer);
return 0;
}

/* sorry I had to repost; in first message the cycle started at 0
incorrectly */

[Isaac Mushinsky]

Re: Code Review: Averaging program

Mark A. Nicolosi wrote:
[color=blue]
> I've been trying to learn C for quite a while. But I've had trouble
> with the
> lack of good quality online text (some of it's alright). But I finally
> bought
> a book on C, Practical C. I like it alot. I'm pretty familiar with the
> C basics,
> because I've been trying to learn it for a while. But anyone, I wrote
> a program
> to average any number of arguments given at the command line. I
> thought it might
> be useful to get feedback about my coding style or anything I've done
> wrong. It
> compile just fine with gcc (with -Wall -ansi -pedantic). Anyways, here
> it is:
>
> /* average.c - Average several numbers together. */
> /* Copyright (C) 2003 Mark A. Nicolosi */
>
> #include <stdio.h>
> #include <malloc.h>
>
> float average (int *numbers, int how_many);
>
> int main (int argc, char **argv)
> {
> int how_many; /* Number of numbers to average. */
> int i; /* Index for for loop. */
> int *numbers; /* Array of numbers to be averaged. */
> float answer; /* Variable to hold the answer. */
>
> how_many = argc - 1; /* Every argument after argv[0] should be
> number to average */
> if (how_many == 0) /* If TRUE then not called with any arguments.
> */
> {
> printf ("Usage: %s <num1> <num2> ...\n", argv[0]);
> return 1;
> }
>
> /* Allocate enough memory to hold the array of numbers to be
> averaged. */
> numbers = malloc (sizeof (int) * how_many);
>
>
> /* Transfer the string arrays in argv[i + 1] to an array of
> integars. */
> for (i = 0; i < how_many; i++)
> {
> sscanf (argv[i + 1], "%d", &numbers[i]);
> }
>
> /* Now that we've got an array of integars to average, average 'em.
> */
> answer = average (numbers, how_many);
> printf ("%g\n", answer);
>
> /* Free the array of numbers. */
>
> free (numbers);
>
> return 0;
> }
>
> float average (int *numbers, int how_many)
> {
> float answer = 0.0; /* Variable to hold the answer. */
> int i; /* Index for for loop. */
>
> /* Loop to add all the numbers in the number[] array. */
> for (i = 0; i < how_many; i++)
> {
> answer = answer + numbers[i];
> }
>
> /* Now divide by the number of numbers. */
> answer = answer / how_many;
>
> return answer;
> }
>
> I copy and pasted that in to links, so I'm not sure if it'll look
> alight :)
> Thanks.
>[/color]

It will work, but
1. better use atoi() than sscanf()
2. There are a lot of unnecessary manipulations, you really do not need to
allocate another array to compute the average. e.g.

#include <stdlib.h>
#include <stdio.h>
int main (int argc, char **argv)
{
int i;
float answer=0;
for (i=0; i < argc; i++)
answer = (answer*i+atoi( argv[i]))/(i+1);
printf (... answer ...);
return 0;
}

[Irrwahn Grausewitz]

Re: Code Review: Averaging program

Isaac Mushinsky <imush@mail.r u> wrote:[color=blue]
>Rather ugly.
>It will work, but
>1. better use atoi() than sscanf()[/color]

Nit: Better use strtol() than atoi(). :-)
<SNIP>
--
6 * 9 = 42 (base 13)

[Ed Morton]

Re: Code Review: Averaging program



Mark A. Nicolosi wrote:
<snip>[color=blue]
>
> /* average.c - Average several numbers together. */
> /* Copyright (C) 2003 Mark A. Nicolosi */
>
> #include <stdio.h>
> #include <malloc.h>
>
> float average (int *numbers, int how_many);
>
> int main (int argc, char **argv)
> {
> int how_many; /* Number of numbers to average. */
> int i; /* Index for for loop. */[/color]

I'd make these unsigned int since they can't be negative.
[color=blue]
> int *numbers; /* Array of numbers to be averaged. */
> float answer; /* Variable to hold the answer. */
>
> how_many = argc - 1; /* Every argument after argv[0] should be
> number to average */
> if (how_many == 0) /* If TRUE then not called with any arguments.
> */[/color]

I'd test for argc == 1 before the first assignment. Just a tiny
efficiency tweak for the error case.
[color=blue]
> {
> printf ("Usage: %s <num1> <num2> ...\n", argv[0]);
> return 1;
> }
>
> /* Allocate enough memory to hold the array of numbers to be
> averaged. */
> numbers = malloc (sizeof (int) * how_many);[/color]

Test for failure, i.e. numbers being NULL.
[color=blue]
>
> /* Transfer the string arrays in argv[i + 1] to an array of
> integars. */
> for (i = 0; i < how_many; i++)
> {
> sscanf (argv[i + 1], "%d", &numbers[i]);[/color]

Instead of incrementing i every time through the loop in argv[i + 1],
either start i at 1 and make the test for <= how_many or move the "i++"
to the first statement within the loop rather than at the end of the
"for..." line.
[color=blue]
> }
>[/color]

<snip>
[color=blue]
> float average (int *numbers, int how_many)[/color]

unsigned int how_many
[color=blue]
> {
> float answer = 0.0; /* Variable to hold the answer. */
> int i; /* Index for for loop. */[/color]

unsigned int i.

<snip>

Regards,

Ed.

[LibraryUser]

Re: Code Review: Averaging program

"Mark A. Nicolosi" wrote:[color=blue]
>[/color]
.... snip ...[color=blue]
>
> /* average.c - Average several numbers together. */
> /* Copyright (C) 2003 Mark A. Nicolosi */[/color]

I wouldn't dare to quote it in full with the above copyright
statement. A more suitable statement for use here might be
"Public domain, by ...".

--
Replies should be to the newsgroup
Chuck Falconer, on vacation.

[Martin Dickopp]

Re: Code Review: Averaging program

LibraryUser <deliberately@m ade.invalid> writes:
[color=blue]
> "Mark A. Nicolosi" wrote:[color=green]
> >[/color]
> ... snip ...[color=green]
> >
> > /* average.c - Average several numbers together. */
> > /* Copyright (C) 2003 Mark A. Nicolosi */[/color]
>
> I wouldn't dare to quote it in full with the above copyright statement.[/color]

In Berne Convention member countries (i.e., almost everywhere), a work
is copyrighted with or without an explict copyright notice.
[color=blue]
> A more suitable statement for use here might be "Public domain, by ...".[/color]

I agree that everyone who posts any example program but the most trivial
should probably explicitly allow people to quote it and try it on their
own computers.

However, the presence or absence of a copyright notice doesn't make any
difference.

Martin

[Irrwahn Grausewitz]

Re: Code Review: Averaging program

Isaac Mushinsky <imush@mail.r u> wrote:[color=blue]
>Rather ugly.
>It will work, but
>1. better use atoi() than sscanf()[/color]

Nit: Better use strtol() than atoi(). :-)
<SNIP>
--
6 * 9 = 42 (base 13)