GNOME Bugzilla – Bug 737007
[PATCH] Remove is_complete().
Last modified: 2014-09-20 17:28:20 UTC
Created attachment 286673 [details] [review] Remove is_complete(). There’s a function I already talked about[1] that I dislike because of how it works, and that does twice the job needed whereas it is called many-many times during the game (used notably by the AI). Here is an elegant way for removing it. [1] https://bugzilla.gnome.org/show_bug.cgi?id=735615
Created attachment 286674 [details] [review] Rework game.vala. Some cleanings. Notably renames place_tile() in try_placing_tile(), as this function is called in blind mode.
Created attachment 286675 [details] [review] Rework game.vala. Ooops, that was a devel patch. Here it should work.
Created attachment 286681 [details] [review] Better import/export. When using the game or even running the tests, we never see the importation/exportation of game managed by [Game]Game.from_strings() and [Game]to_string(). But sometimes, when debugging, we need to have a function that prints the game in console in a readable way. And I hate having to recode it each time. So here is a patch that makes to_string() human-readable (and helps reading test-iagno.vala).
Review of attachment 286673 [details] [review]: Can you rebase this series please? (Sorry!) I like this approach, but instead of removing is_complete(), just leave it as a wrapper around can_move(null), since game.is_complete() is more understandable than game.can_move(). ::: src/game.vala @@ +161,3 @@ } + public bool can_move (Player? color = null) Then you can consider not adding the default argument here, since I'd rather we always call game.is_complete() instead.
Review of attachment 286675 [details] [review]: ::: src/game.vala @@ +189,3 @@ \*/ + public int try_placing_tile (int x, int y) Don't rename this. What if every function that could fail was named try_doing_something()? You could make it throw a checked exception if you want. That would be better for the tests, too. @@ +254,3 @@ xt += x_step; yt += y_step; + } while (is_valid_location (xt, yt) && tiles[xt, yt] == enemy); This is wrong!
Review of attachment 286681 [details] [review]: OK, but please split this into two patches and post them both in a new bug.
Created attachment 286694 [details] [review] Rework is_complete(). (In reply to comment #4) > I like this approach, but instead of removing is_complete(), just leave it as a > wrapper around can_move(null), since game.is_complete() is more understandable > than game.can_move(). That won’t help my little mind, but at least the code won’t do unnecessary works. Here’s the patch.
Closing, but note the two unpushed patches! Attachment 286694 [details] pushed as 8c9c480 - Rework is_complete().
Review of attachment 286675 [details] [review]: ::: src/game.vala @@ +254,3 @@ xt += x_step; yt += y_step; + } while (is_valid_location (xt, yt) && tiles[xt, yt] == enemy); Just kidding, the comment "Count number of enemy pieces we are beside" is wrong.