Created attachment 284724 [details] [review]
Little things in game.vala.

Here is a second patch with many little things I wanted to do in game.vala…

Created attachment 284747 [details] [review]
Little thing in computer.vala.

One more patch.

Review of attachment 284723 [details] [review]:

I'm not really sure about the semantics of this.  Overloading the Player enum to indicate whether a position is valid or not doesn't seem very intuitive.  Now if the enum was named Position, I'd be more enthusiastic.

::: src/game.vala
@@ +106,2 @@
     public Player get_owner (int x, int y)
     {

I'd rather add a contract to this method: requires (is_valid_location(x, y)).  It looks like computer-player.vala already ensures the location is valid.

Review of attachment 284724 [details] [review]:

::: src/game.vala
@@ +29,3 @@
      * and yyy is the y location (0-7).  Each set of changes is followed by the number of changes
      * preceeding.  This array is oversized, but big enough for the (impossible) worst case of
+     * each move flipping 20 tiles. TODO Support lenght>8's boards, as the rest of the 'lib'. */

typo :)

@@ +74,3 @@
+
+        if (width != 8 || height != 8)
+            return;

Then I guess nobody can ever make the first move?

I'd rather go the opposite way on this: make height and width static constants rather than arguments. The code will never be used for any board other than 8x8.

@@ +79,1 @@
         set_tile (3, 3, Player.LIGHT, false);

You could do something like 

set_tile (WIDTH/2 - 1, HEIGHT/2 - 1, ...)
set_tile (WIDTH/2 - 1, HEIGHT/2, ...)

etc.

@@ -112,3 @@
         ensures (result || n_tiles < width * height)
     {
-        return !can_move (Player.LIGHT) && !can_move (Player.DARK);

Why did you change this?  The original version is simpler.

Review of attachment 284747 [details] [review]:

::: src/computer-player.vala
@@ +268,3 @@
 
+        if (moves == null)
+            critical ("random_select() shouldn't be called when no move's available");

It'd be nicer to use warn_if_reached() or assert_not_reached() rather than writing a custom message.  Decide which one to use based on how confident you are that you're correct. :)  assert_not_reached() will result in bug reports if you're wrong, which may or may not be a good thing.

Created attachment 284838 [details] [review]
Return value that didn't make sense.

(In reply to comment #3)
> Now if the enum was named Position, I'd be more enthusiastic.

No, you won’t. Because you don’t want to have in game-view.vala:
    var winning_color = Position.LIGHT;
    var losing_color = Position.DARK;
The only other name I have in mind for a so globally used enum is “Color”.

> I'd rather add a contract to this method: requires (is_valid_location(x, y)). 
> It looks like computer-player.vala already ensures the location is valid.

Yes, that’s one of the others solution I had in mind, here’s the associated patch.

Created attachment 284842 [details] [review]
Little things in game.vala.

(In reply to comment #4)
> +     * each move flipping 20 tiles. TODO Support lenght>8's boards, as the
> rest of the 'lib'. */
> 
> typo :)

Not adding this text doesn’t mean it’s no more in my todo-list, you know? :þ

> +        if (width != 8 || height != 8)
> +            return;
> Then I guess nobody can ever make the first move?

Mh, right, forgot that set_tile() is private. The only way to init to a different position might be to call from_strings(), for now. But I want to have a way at the end to play the second reversi’s position:
. . . . . . . .
. . . . . . . .
. . . . . . . .
. . . O X . . .
. . . O X . . .
. . . . . . . .
. . . . . . . .
. . . . . . . .

> The code will never be used for any board other than 8x8.

Variant on 10×10 exists. If we could maintain a generic code, it would be quite interesting. And not so complicated (apart the history thing…), all is already done.

> You could do something like 
> 
> set_tile (WIDTH/2 - 1, HEIGHT/2 - 1, ...)
> set_tile (WIDTH/2 - 1, HEIGHT/2, ...)

I’ll do that, just needs a check that the board length & height is even.

> Why did you change this?  The original version is simpler.

I don’t think it’s really simpler to call a function that calls a function that calls a function… It’s difficult to have all the structure of the code in mind with that; I prefer that a `grep` tells me exactly where the can_move() function is completely useful (returning if a particular player can move…), than having some corner-case calls like this one (testing if someone can move, by testing all the players one after the other). The code I suggest is clear and works better (only one loop), it would quite help me if it was used.

(In reply to comment #5)
> It'd be nicer to use warn_if_reached() or assert_not_reached() rather than
> writing a custom message.  Decide which one to use based on how confident you
> are that you're correct. :)  assert_not_reached() will result in bug reports if
> you're wrong, which may or may not be a good thing.

It’s more an information about how the function works, for developers, than a thing that could really “bug”. I mean, should I call the function random_select_when_move_available()? or split in two functions, one for checking the moves and one for random? I think the “comment in console” way is the nicer, but I don’t really know. Of course, if one of the AI uses random moves, we would need a more solid function…

Review of attachment 284842 [details] [review]:

::: src/game.vala
@@ -113,3 @@
         ensures (result || n_tiles < width * height)
     {
-        return !can_move (Player.LIGHT) && !can_move (Player.DARK);

I still don't understand why you changed it. Writing it out the long way is not more clear. :p  The game is over if neither player can move; otherwise, it's not over.  This one-liner is exactly equivalent to your two for loops, two conditionals, and two return statements.

The rest is fine.

Review of attachment 284838 [details] [review]:

::: src/computer-player.vala
@@ +255,1 @@
     private static int is_empty (Game g, int x, int y)

Surely this should be changed to a bool?

@@ +255,3 @@
     private static int is_empty (Game g, int x, int y)
     {
+        return x < 0 || x >= 8 || y < 0 || y >= 8 || g.get_owner (x, y) != Player.NONE ? 0 : 1;

This is too messy for me, but I think it'd be fine if the return value was a bool (as then you wouldn't need the ternary).

(In reply to comment #7)
> I think the “comment in console” way is
> the nicer, but I don’t really know.

It's just a long way of saying "this should never be null" -- that's common enough that writing a custom message each time would just be clutter.

Created attachment 284864 [details] [review]
Return value that didn't make sense.

> This is too messy for me, but I think it'd be fine if the return value
> was a bool (as then you wouldn't need the ternary).

Impossible, see the use case of this function. I just didn’t touch anything there, that’s sometimes simpler. =)

> I still don't understand why you changed it. Writing it out the long way is not
> more clear. :p  The game is over if neither player can move; otherwise, it's
> not over.  This one-liner is exactly equivalent to your two for loops, two
> conditionals, and two return statements.

It’s half the number of loops (the board is only checked once), for a very used function…

Created attachment 284865 [details] [review]
Little thing in computer-player.vala

(In reply to comment #10)
> It's just a long way of saying "this should never be null" -- that's common
> enough that writing a custom message each time would just be clutter.

Yeah, why not. Here’s a corrected patch.

Review of attachment 284864 [details] [review]:

OK

Review of attachment 284865 [details] [review]:

OK

(In reply to comment #11)
> It’s half the number of loops (the board is only checked once), for a very used
> function…

That's true. If you think this optimization is worth the additional code complexity, you can support it with results from gprof.  (But again, I don't understand why we care about running time at all, when it's so fast we have to inject artificial delay. But I've mentioned this before. :)

The other thing we should do would be to run the AI in a separate thread, so the UI doesn't block while it runs (briefly as it does).

Note that there's one patch that has not been pushed!

Attachment 284864 [details] pushed as 674ef43 - Return value that didn't make sense.

Are you going to update the final patch?

I have merged all but the optimization in a patch for having a --size option, that I just need to rebase. Didn’t find for now how I will make is_complete() work correctly. :þ

OK then!