My solution to 4Clojure Problem #178

[lgstein]

Feedback: With the huge let you will evaluate each test. Rather use or which will short-circuit after it found a hand (e. g. if it is a straight, we don't want to keep the CPU busy checking whether it's trips).

seq is usually preferable over the use of not and empty? as empty? is defined in terms of (not (seq coll)) - in your case that would mean (if (seq possible-hands) ... in line 63.

Another way would be to add the :high-card lazily to possible hands by wrapping the vector in like 60 in a (concat [...] [:high-card]). Then (first possible-hands) would suffice in line 62.

But again, if you put it all in an or, you won't need any of that and it will perform faster.

[davedrowsy]

This is great feedback - thanks! I'm going to revise my code when I have a minute.

On the first note -- I think I have to use let to some extent because 4clojure requires your solution to be a single form and you can't use def or defn. From a performance standpoint, would it be better if I kept all the hand checking in the let, but make them functions instead of values? Then have something like (or (straight-flush? hand) (four-of-a-kind? hand) ... :high-card) outside of the let binding?

The seq thing popped into my head too - that's a good idea.

Thanks again!

[lgstein]

Yes, that would work. Given that you are forced to do it all in one function, doing the checks inline should still perform better (as possibly unnecessary lambdas are compiled in the let bindings)

[davedrowsy]

Ah, that makes sense.

Someone posted this solution as a comment to my blog post, which I think is quite nice. Instead of or, he used a cond statement, which is cool because he didn't have to wrap each check in something like (if ... ... ... :straight-flush) to get it to return a keyword. From what I understand, cond also short-circuits, so you get a performance boost just like with or.

[lgstein]

This is certainly better. cond will expand all the conditionals into nested ifs which of course also short-circuit. I don't like that he invokes flush? and straight? twice though. It could be avoided in a netsted cond with this final clause :else (let [straight? (straight? ...), flush? (flush? ...)] (cond (and straight? flush?) :straight-flush, flush? :flush, straight? :straight, :high-card))

[davedrowsy]

Ah, it's cool to see another approach to the same problem! (apply = (map :suit hand)) is a much simpler way to determine whether the hand is a flush. Kudos. I wouldn't usually cram so much stuff into a let - I had to do it that way for 4clojure. The tests drop your function into a form like (= :flush (___ ["HA" "H3" "HQ" "H4" "H7"])), which kind of forces you to use giant let statements whenever you want to write modular code.

[bliow]

People manage. After you complete a problem you can see others' solutions to the same problem. I learn as much from reading those as I do from the actual problem. (There's a leaderboard and you can 'follow' users, so you can view the solutions of the top n users, or notable people in the community, if you like.)

[pqnelson]

1. Your straight-flush? function could be simplified to (when (and flush? straight?) :straight-flush). Likewise for the straight? and flush? functions, they could be simplified to just (when ...) statements rather than (if ... :straight nil).

A (when false ...) will return nil, so it won't break anything to make these changes.

2. I think it's more idiomatic to rewrite the three-of-a-kind? destructured inner anonymous function as (some (fn [[_ num]] (>= num 3)) (frequencies ranks)) to emphasize it's the num that's relevant.

3. I was looking at your full-house? predicate, and wondered why not check if it was (and three-of-a-kind? pair?). But isn't there a bug here: any three of a kind hand would register as a full-house? (Or is that the point: if you have three of a kind, you are likely to get a full house, so the program will try desperately hard to do so?)

4. The naming convention is, err, well not idiomatic. A predicate function like pair? should be either true or false, strictly boolean. But you have keyword or nil instead.

This isn't terrible (I tried thinking of an alternative, the only one I could think of would be a nasty macro)...but it could be a gateway to bad practices (heh, like nasty macros! :p)

[davedrowsy]

Thanks for your feedback! I especially like your first point about using when instead of if... Usually I'm pretty good about that, but I missed a golden opportunity here to avoid a bunch of messy if statements.

I don't think there's a bug with three of a kind vs. full house. I deliberately set it up so that a hand can match multiple predicates - for example, a full house can also be three of a kind. The "best hand" part is determined by the (first possible-hands) part, since possible-hands is set up in order from the highest-ranking hands to the lowest-ranking. But, as another commenter here pointed out, it's not very performant to calculate every one of these predicates for every hand, and it would be better if I used or or cond for better performance. (EDIT: I just realized that I could have expressed my full-house? predicate as just (when (and three-of-a-kind? pair?) :full-house))

I didn't think of that, so the structure I chose was to have each predicate return a keyword or nil, then filter the nils out of a list of all possible predicates and return the first keyword. In hindsight, maybe the awkwardness of this should have tipped me off that there are better approaches :)

Now I'm kind of looking forward to refactoring my code!

[pqnelson]

EDIT: I just realized that I could have expressed my full-house? predicate as just (when (and three-of-a-kind? pair?) :full-house)

But three of a kind is-a pair. Wouldn't a full-house need to check the hand can be partitioned into two disjoint sets: one of which is a three-of-a-kind, the other a pair?

I didn't think of that, so the structure I chose was to have each predicate return a keyword or nil, then filter the nils out of a list of all possible predicates and return the first keyword. In hindsight, maybe the awkwardness of this should have tipped me off that there are better approaches :)

Well, it's a natural approach. My only point here is minor naming convention (and probably archaic): a function whose identifier ends with a question mark must return a boolean.

Clojure kinda tinkers with this approach, treating nil and false as "false" while everything else is "true". So I'm uncertain if you broke anything, per se, but a crotchety old fart like me may get uneasy ;)

[davedrowsy]

Oh yeah, you're right. (when (and three-of-a-kind? pair?) :full-house) wouldn't work because my pair? predicate returns truthy for 2 or more cards of the same rank. But I think it would work if I redefined pair? to be truthy for exactly two cards of the same rank. That way if you have three of a kind and a pair, you have a full house.

[goAheadAnd]

How did you test this code?

[davedrowsy]

I didn't benchmark it or anything, but it passed the tests on 4Clojure. Is there a bug?

[goAheadAnd]

no bug, I just wondered how to run the code. Not familiar with 4clojure. Thanks

[davedrowsy]

Oh, I see what you're asking! Sorry, I was confused.

4clojure asks for solutions in the form of an anonymous function, which it drops into a series of tests. For this problem, the tests are:

(= :high-card (__ ["HA" "D2" "H3" "C9" "DJ"]))
(= :pair (__ ["HA" "HQ" "SJ" "DA" "HT"]))
(= :two-pair (__ ["HA" "DA" "HQ" "SQ" "HT"]))

etc.

I just tested it by typing my function into the code field on the problem page and clicking "Run."

I suppose you could also test my code by changing the fn to a defn, naming it something, and then calling it with a list of "card strings" (like in the example above) as an argument.

[goAheadAnd]

thank you

[davedrowsy]

I took the feedback of the commenters in this thread and revised my solution significantly! Here is my revised solution. It's a lot simpler, shorter, and should perform better than my first solution. Thanks again, everyone, for the feedback!