--- "johnmatthews2000" <jm5...@...> wrote:
>
> 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))
What is the precedence between && and ! ??
I need to look for it in the standard.
> 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'.
You are right.
> 4. Not enough comments (none!).
I assume that my students and users are smart ;)
> 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).
The operator & consumes less CPU cycles than the operator % .But you are
right: The CPUs today are so fast that none should care about a few extra CPU
cycles.
> 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
It is const, but in the future it may be a variable.
> 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.
You are right.
> 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.
I'll look in the generated Assembly.
> 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.
It is a feature: It causes the user to compete against his previous
performance.
> 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.
I use a dual boot machine and I want to share the statistics file. So it must
be compatible between architectures.
> 11. All your functions, apart from main(), should be static.
If someone decide to link against it, he will probably need them.
> 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
You are right.
> 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.
I'm thinking to parse a CL argument for valuemax.
P.S.: The wallpaper's glue's solvent is making me crazy.
