GNOME Bugzilla – Bug 659623
New game: Hamilton
Last modified: 2015-06-08 20:37:38 UTC
Created attachment 197091 [details] [review] complete implementation and documentation for new game Hamilton Back in 1999, according to my notes, I started implementing my favourite solitaire game for aisleriot. A dozen years later, I got round to finishing it. (In the intervening years, among many other happenings, I finally finished reading SICP and implemented my own Scheme interpreter, so I'm confident that the code is of a reasonable quality.) Hamilton is a single-pack game of skill, rather similar to spiderette (although not as hard), and it has a unique twist. The game has some history: I learned it as a child in the 1970s, and I have found references to it in "Games of Patience" by Basil Dalton (London 1924) and "Teach Yourself Card Games for One: Patiences - Solitaires" by George F Hervey (London 1965). I've followed the instructions in games/README, so I hope the attached patch (made against the current git head of 5db5650f8746..) is complete and correct.
Thanks for the complete patch! The non-scheme parts look good to me; I'll let Vincent review the scheme code.
I always worry when someone says they know scheme, because it usually means they use all kinds of maps and lambdas and reduces that I have to think really hard to figure out. But you didn't do that until cartesian-product. I'm just going to assume that function is accurately-named and reliable. Same with get-moveable. Some nit-picky things: * If I double-click on a tableau pile that cannot be moved to the foundation, the button-double-clicked function will return a true value but do nothing, adding an empty step to my undo history. It doesn't look like it should do this, but it turns out the function is missing an "else", which I guess makes cond behave strangely. * You need to mark the strings users will see for translation, like this: (_"string"). * I can't wait to hear what our translators will have to say about the string in "(list 2 (get-name (get-top-card chooser)) "a foundation, or flip over the next card of the deck")". The "Move X onto Y." format is pretty broken for translators already, and now Y includes a whole extra sentence? I suggest hinting one thing or the other. If users want to know TWO things they can do, they can read the help. * You defined (dealable?) and (deal) but didn't set the dealable feature. Don't you want the pretty deal toolbar icon we (and by we I mean Christian Persch) worked so hard on? * If you can move multiple cards from a tableau slot to a foundation, the game will tell you to move the highest-ranked card in the sequence, instead of the lowest card. I find this confusing, as moving multiple cards to a foundation is generally considered a shortcut for a sequence of single legal moves, not a single move.
> I always worry when someone says they know scheme :-) IMHO clarity always comes first... > * If I double-click on a tableau pile that cannot be moved to the foundation, Well spotted! (The value of a cond with no match is unspecified.) > * You need to mark the strings users will see for translation, like this: D'oh! I knew that, of course. > * I can't wait to hear what our translators will have to say about the string Yes, you're quite right. Hints should offer a single good (or possible) move, and not try to explain the rules. > * You defined (dealable?) and (deal) but didn't set the dealable feature. Ah, sorry about that. In my defence, the aisleriot / guile interface isn't documented as well as it could be. (Maybe I should fix that!) > * If you can move multiple cards from a tableau slot to a foundation, the game > will tell you to move the highest-ranked card in the sequence, instead of the > lowest card. I find this confusing, as moving multiple cards to a foundation OK, I agree with that point. Interestingly, my first implementation had, effectively, two sets of rules embedded in the guile code: one to handle actual moves, and a second to handle hints. That version produced "single card" hints. I realised that the code would be much cleaner (and shorter and more likely to be correct) if there were a single set of rules, and the hints made use of functions like droppable?. And when I implemented that, the "multi card" hints fell out for free, so I stuck with them. Anyway, thanks very much for the code review, Vincent. Would you like me to incorporate your comments into a new patch? #t
It's up to you, really. If you don't, I'll (eventually) commit it with fixes.
Created attachment 289240 [details] [review] updated hamilton patch Here's an updated patch against a recent HEAD (specifically b6eb1a74fb5) As before, it's a complete documented implementation. I've updated it for changes to the aisleriot api, and I've fixed all the problems Vincent mentioned. Let me know if there's anything else you need. Cheers, Toby.
Oh geez, I said I'd fix and commit this eventually, but clearly I never got around to it. Sorry about that. Hopefully I can make up for it this time.
(In reply to comment #6) > Oh geez, I said I'd fix and commit this eventually, but clearly I never got > around to it. Sorry about that. Hopefully I can make up for it this time. No worries. :-) I really appreciate all your hard work on aisleriot. (If I didn't have at least half a dozen other spare-time open source projects of my own, I wish I could help out...)
Do we need to do something to make "chooser" as a slot name translatable?
Created attachment 289826 [details] [review] game: Add 'chooser' slot type
The patch looks good to me, and it survived both automated and manual testing.
Ok to commit :-)
Bump. I'm not seeing the patch in the git repo. Anything I can do to help it happen? Thanks :-)
Comment on attachment 289240 [details] [review] updated hamilton patch As per comment 10.
Pushed to master.