[pio-develop] Code review genetic player

[Roland C. <rc...@us...>]

Hallo Rodrigo and list,

The genetic computer player is playing better than the greedy player.
My test results so far in the default game, with 2 classic computer
players and 2 genetic players: classic vs. genetic = 1 vs 4 won games.

I've done a code review of the new code. Most are relatively minor
issues, but will make maintenance easier.

- Could you run 'make reindent' before committing to svn? That will
minimize the amount of changes. (Your editor appears to use a tab size
of 4, while the other source code assumes a tab size of 8).
- I've already run 'make reindent' and committed the changes, so I
suggest you run 'make reindent' before 'svn update', otherwise you'll
get many nasty merge conflicts.
- Try to keep the amount of changes small, run 'svn diff' to see if you
are about to commit some unneeded white space changes.
- Try to avoid magic numbers. In
reevaluate_gameState_supply_matrix_and_resources I see '10' and '5'. You
can use the GLib macro 'G_N_ELEMENTS', In outputChromosome you use the
value of 9. Try to use either 9 or 10.
- Use the datatypes in GLib, especially gboolean and TRUE/FALSE, that
makes the code easier readable (see e.g. facingOK)
- Avoid the pattern 'if (A) { return TRUE } else { return FALSE }', use
'return (A)' instead.
- Use a typedef for your structs and enums, so you don't have to use the
keyword 'struct' or 'enum' in every function call.
- In facingOK: 'nodeTwo = hex->nodes[(hex->facing + 5) % 6]' does the
same as the if-statement
- Instead of keeping your template code around in '#if 0', remove it
when you are done with it.
- Instead of 'printf', use GLib's 'g_print' (or g_printf). This will
remove the need to include stdlib.h and stdio.h
- Instead of 'sleep' use 'ai_wait', or if you need only one millisecond,
use 'g_usleep(1000u)' (I've replaced the call to sleep with 'ai_wait',
because the sleep function broke the Windows build).
- In checkRoadNow: It is not necessary to place a break after a return
statement.
- If you intend to keep the code in genetic_core.c separate from
genetic.c (so you might use it with another main(), I suggest to create
a regular 'genetic_core.h' file to contain the function prototypes. If
you have trouble integrating it in the Makefiles, contact me.
- genetic_game_over: You can use the argument points instead of
'player_get_score(my_player_num())' (and thus remove the G_GNUC_UNUSED
for points)
- Instead of commenting and de-commenting your debug messages, you can
add '#define GENETIC_DEBUG' and '#ifdef GENETIC_DEBUG...#endif'

I've prepared a patch [1], which should be applied to svn revision 2066,
which removes all duplicated code related to the chat-behaviour of the
computer player to the main code file (ai.c).
I've noticed that this might break your code by introducing many merge
conflicts, so I'll apply it only when you agree.

With kind regards,
Roland Clobus

[1] https://sourceforge.net/p/pio/patches/677/