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

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

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

 

[Kevin Easton]

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

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

malloc.h is a non-standard header - to get a declaration of malloc, you

float average (int *numbers, int how_many);

In general, you should choose "double" for floating point numbers,
unless you have a specific reason for choosing "float".

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

It is possible for argc to be zero, in which case this if branch will
not be taken.

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

....which is a good thing, because in that case argv[0] would be NULL and
you would have a problem here.

return 1;
}

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

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

/* 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);
}

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

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

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

return answer;
}

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]

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

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

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

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+1);
printf (... answer ...);
return 0;
}

 

[Kevin Easton]

I wrote:
[...]

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

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

while (*++argv) {

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]

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;
printf ("answer %f\n", answer);
return 0;
}

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

 

[Isaac Mushinsky]

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

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

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

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+1);
printf (... answer ...);
return 0;
}

 

[Irrwahn Grausewitz]

Rather ugly.
It will work, but
1. better use atoi() than sscanf()

Nit: Better use strtol() than atoi().
<SNIP>

 

[Ed Morton]

Mark A. Nicolosi wrote:

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

I'd make these unsigned int since they can't be negative.

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

I'd test for argc == 1 before the first assignment. Just a tiny
efficiency tweak for the error case.

{
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);

Test for failure, i.e. numbers being NULL.

/* 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);

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.

}

float average (int *numbers, int how_many)

unsigned int how_many

{
float answer = 0.0; /* Variable to hold the answer. */
int i; /* Index for for loop. */

unsigned int i.

<snip>

Regards,

Ed.

 

[LibraryUser]

.... snip ...

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

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

 

[Martin Dickopp]

I wouldn't dare to quote it in full with the above copyright statement.

In Berne Convention member countries (i.e., almost everywhere), a work
is copyrighted with or without an explict copyright notice.

A more suitable statement for use here might be "Public domain, by ...".

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

 

[Default User]

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

As Martin pointed out, copyright applies whether this statement appears
or not. However, it's immaterial as quoting is allowed for the purpose
of commentary or criticism (Fair Use).




Brian Rodenborn

 

[Neil Cerutti]

As Martin pointed out, copyright applies whether this statement
appears or not. However, it's immaterial as quoting is allowed
for the purpose of commentary or criticism (Fair Use).

Uh oh.

Is there a standard C function that can help detect and prevent
copyright threads? ;-)

 

[Ben Pfaff]

Is there a standard C function that can help detect and prevent
copyright threads? ;-)

Standard C does not support threads. You will have to use a
platform-specific library, and as such you should consult a
platform-specific newsgroup.

 

[Tom Zych]

Is there a standard C function that can help detect and prevent
copyright threads? ;-)

I think gcc includes a function that will convert copyright threads
to copyleft threads. However, being cross-threaded, it will only
work with cross-compilation.

 

[LibraryUser]

In Berne Convention member countries (i.e., almost everywhere), a work
is copyrighted with or without an explict copyright notice.


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.

Maybe I am excessively subtle. I was trying to point out the
foolishness of the copyright statement in the first place.