[OCLUG-devel] ArrayMaxValue() - how could I improve it?

[Christopher Smith]

Okay, a couple of things:

1) Your book is probably based on the C89 standard, rather than C99, so
it probably doesn't mention C++ style comments, but I'd encourage their
use in a number of places

2) There is no need for the number parameter. Consider this pseudo code:

int array[20] = { /* initialize the array */ };

int answer1 = ArrayMaxValue(0, array);
int answer2 = ArrayMaxValue(1, array);
int answer3 = ArrayMaxValue(123124, array);

Would you expect answer3 be any different than answer1 or 2? No. This is
a good indicator that it should not be a function parameter. Technically
the variable isn't needed at all, but it does help with code clarity, so
I wouldn't discard it out of hand.

3) Your while loop test condition is broken. "array[position]" returns
the value stored at offset "position" in the array "array". What you
want is just "position". Also, because array indexes are zero based, you
want just "<" instead of "<=".

4) Currently, your "position" never changes, so your loop will never
exit, and your function will never return. Sometimes it is easier to use
a "for" loop to avoid this kinds of errors when iterating through an
array.

5) Always declare the return type of your functions. While C89 will
default to the return type of an int, C99 will not. More importantly it
is just good syntactic hygiene.

6) I'm not a big fan of the "no braces block", particularly when you
have comments which kind of make it look like there is more than one
line there. That being said, there are those who would say I was flat
out wrong, so consider this just a matter of style.

7) You don't need parenthesis around MaxValue in your return statement.

8) I find it helpful to follow consistent caps rules for my variables.
So, either "position" and "MaxValue" should both be capitalized, or they
both should start with a lower case letter, but not a mix.

9) Most Unix C hackers use underscore characters for word boundaries in
names, rather than changes in capitalization. So, it'd be
"array_max_value" instead of "ArrayMaxValue". This is also very much a
stylistic thing, but it is worth noting that ArrayMaxValue is the
Windows style. ;-)

10) I'd encourage you to use tools like doxygen for your code
documentation.

11) Your current design passes in the array by value, rather than by
reference. I assume you haven't reached the section on pointers in your
book yet, so I won't go in to how you would make it pass by reference,
but suffice it to say, you'll be discovering better ways to do this
shortly.

12) Your function could tack on another parameter which indicates the
number of elements in the array to examine.

13) array[] doesn't hold a *maximum* of 20 elements, it holds exactly 20
elements. There is no magic in C that indicates you are at the end of an
array. If you called it with an array of 19 elements it would corrupt
the stack.

14) Since your array is made up of *signed* integers, it would be a bad
assumption that your maximum value will be greater than or equal to zero
(what if the array is all negative values?). Why not just initialize it
with the first element of your array?

Attached is a sample implementation of your function that I came up
with. I tried not to use concepts you might not understand or have
familiarity with. See if it makes some of what I'm saying any clearer.

-- 
Christopher Smith <x at xman.org>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: maxValue.c
Type: text/x-csrc
Size: 623 bytes
Desc: not available
Url : http://localhost.localdomain/pipermail/oclug-devel/attachments/20040603/50219fb9/attachment.bin