--- In [email protected], "izecksohn" <izecks...@...> wrote:
>
> I think the C code is perfect now. Could you criticize it again?
Ok - and these are only my opinions, in no particular order; feel free to argue
:-)
1. Not code, but I'm going to mention it anyway. If I make the program on the
Linux laptop, I get the following error:
make: *** No rule to make target `rebase.exe', needed by `rebase.zip'. Stop.
I guess this is a cygwin issue - perhaps you could provide different makefile
targets eg.
make unix
for unix/linux (doesn't build/require rebase.exe) and
make cygwin
for cygwin (builds/requires rebase.exe).
(And I still don't think you should be building the .tar and .zip files in the
default 'all' target.)
2. Although most of the code looks ok, you don't use spaces within expressions,
which for most people makes them harder to read and understand. For example:
if ((!st->f)&&(ENOENT==errno))
if ((fscanf_returned<1)&&(!feof(st->f)))
These would be better written:
if (!st->f && (ENOENT == errno))
if ((fscanf_returned < 1) && !feof(st->f))
3. In main():
int base = 10; /* << redundant assignment */
do
{
base = ((rand()%35)+2);
} while (10==base);
The assignment in the declaration of base is redundant; whatever you set it to
will be overwritten by the first iteration of the 'while' loop.
This also applies to other variables eg. 'entered', 'seconds'.
4. Not enough comments (none!).
5. In main():
const int valuemax = 0x7F;
int value = (rand()&valuemax);
Presumably the purpose of these statements is to generate a (psuedo-)random
number in the range 0..127. However, because you use the bitwise-AND operator,
it isn't safe. That is, if someone else came along in the future and decided to
extend the range to 0..128, they might change the code to:
const int valuemax = 0x80;
int value = (rand()&valuemax);
But as I'm sure you can see, this will only result in 2 values - 0 or 128. So,
either use comments:
/* Generate test value. */
/* NB. valuemax MUST BE (POWER OF 2) - 1 */
or (better) use statements that don't depend on the value of valuemax to work
correctly eg. use the remainder (%) operator, like you do to generate the
number base value (see below).
6. Constants such as valuemax are usually defined at the top of the code, and
because this is a C program, I would expect it to be a macro eg.
:
#include <string.h>
/* Range of values user will be asked to convert. */
#define TEST_VALUE_MIN 2
#define TEST_VALUE_MAX 100
:
int main(..
:
/* Generate the 'random' test value. */
int value = (rand() % (TEST_VALUE_MAX + 1 - TEST_VALUE_MIN)) + TEST_VALUE_MIN;
7. You don't have to define buflen; you could just use:
char buffer[34];
:
.. fgets(buffer, sizeof buffer, stdin);
This is safer, because the person checking your code can immediately see that
the above code is ok just by looking at the fgets() statement. However with:
..fgets(buffer, buflen, stdin);
the person has to go back to the definition of buflen and buffer to check that
buflen == sizeof buffer.
8. You use 'do {..} while (1)' for your infinite loop; 'for (;;) {..}' is
generally considered 'better' (eg. by PC-lint) because it doesn't require an
expression, although of course you would expect the expression to be optimised
out; it's just a style thing.
9. You print out the user's average time before calling statistics_append, so
the printed average doesn't include the last result. I don't know if this is
deliberate, but it looks like a bug.
10. In the fopen() statement you use mode "rb+"/"wb+". Although "the âbâ is
ignored on all POSIX conforming systems, including Linux" (Linux fopen man
page), it implies you are opening a binary file. But the statistics file is a
text file, so you shouldn't use 'b' here.
11. All your functions, apart from main(), should be static.
12. It should be:
static void statistics_init (statistics *st, const char *filename);
because the filename string isn't modified by the function. Without the const,
the following would result in a compiler warning, but it shouldn't:
const char filename[] = "rebase_stats.txt";
statistics_init(&st, filename);
rebase.c:43: warning: passing argument 2 of âstatistics_initâ discards
qualifiers from pointer target type
13. Just a suggestion, but perhaps you could improve the program by making it
easier/harder depending on the user's results. For example, you could make the
test value range a variable instead of constant, and increase the range if the
average time is low, or decrease the range if the time is high.
Cheers
John
