HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Poker Hands Kata in F#

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
pokerkatahands

Problem

I'm a F# newbie, and would like to share my implementation of the Poker Hands Kata problem to get some feedback.

```
module PokerHands

open System.Text.RegularExpressions

type Figure =
| Two | Three | Four | Five
| Six | Seven | Eight | Nine
| Ten | Jack | Queen | King | Ace

type Suit = Diamonds | Spades | Hearts | Clubs

type Card = Figure * Suit

type Rank =
| HighCard of Figure * Rank option
| Pair of Figure * Rank
| TwoPair of Figure Figure Rank
| ThreeOfKind of Figure
| Straight of Figure
| Flush of Rank
| FullHouse of Figure
| FourOfKind of Figure
| StraightFlush of Rank

type Player = {
Name: string;
Cards: Card list;
Hand: Rank;
}

let SortDesc sequence = sequence |> Seq.toList |> List.sortWith (fun x y -> compare y x)
let SortDescBy f sequence = sequence |> List.sortWith (fun x y -> compare (f y) (f x))

let rec BuildHighCard (figures : Figure list) =
match SortDesc figures with
| [h] -> HighCard(h, None)
| h :: t -> HighCard(h, Some(BuildHighCard(t)))
| _ -> failwith "empty"

let isFlush (cards : Card list) =
cards |> Seq.distinctBy snd |> Seq.length = 1

let isStraight (cards : Card list) =
let figs = cards |> List.map fst |> List.sort
let flag = ref true
for i in {1 .. List.length figs - 1} do
if (compare figs.[i-1] figs.[i]) <> -1 then flag.Value List.map fst))) else None

let (|IsFlush|_|) cards =
if isFlush cards then Some(Flush(BuildHighCard(cards |> List.map fst))) else None

let (|IsStraight|_|) cards =
if isStraight cards then Some(Straight(cards |> List.minBy fst |> fst)) else None

let (|IsGrouped|_|) (counts : int list) cards =
let groups = cards |> Seq.countBy fst |> Seq.toList |> SortDescBy snd
if groups |> List.map snd = counts then Some(groups |> List.map fst) else None

let DetermineRank (cards : Card list) =
match cards with
| IsStraightFlush straightFlus

Solution

I like this! Your use of discriminated unions and active patterns is quite elegant, I think. (Although exploiting that compare on discriminated unions does what it does seems a bit of a hack–although I have a hard time justifying that sentiment.)

As to your concerns:

-
Here is a manual way to do it functionally (replace the let flag ... flag.Value with this):

let rec check = function 
        | x :: y :: zs -> compare x y <> -1 && check (y :: zs) 
        | [x] -> true
        | _ -> false
check figs


For more on this style, see here. Alternatively, you could try to use combinators. If you're willing to compare only sequences of five cards, then maybe like this?

List.toSeq cards |> Seq.distinct |> Seq.toList 
    |> function
       | [x;_;_;_;y] -> compare x y = -4
       | _ -> false


-
I think these are fine. I don't think you should try to factor out the redundant looking BuildHighCard(cards |> List.map fst). I think if you did, you'd clutter your program rather than make it more readable. As it is, it's perfectly clear what's going on, yes?

-
Yes:

let winner, loser = Array.max players, Array.min players


But maybe already ParsePlayers return a tuple instead of an array?

Minor suggestion: You could implement SortDesc and SortDescBy as:

let SortDescBy f = List.sortWith (fun x y -> compare (f y) (f x))


In general, when you write x |> f it means just f x. So when you define

let f x = x |> g ...


you can cut out the middle man and just do

let f = g ...

Code Snippets

let rec check = function 
        | x :: y :: zs -> compare x y <> -1 && check (y :: zs) 
        | [x] -> true
        | _ -> false
check figs
List.toSeq cards |> Seq.distinct |> Seq.toList 
    |> function
       | [x;_;_;_;y] -> compare x y = -4
       | _ -> false
let winner, loser = Array.max players, Array.min players
let SortDescBy f = List.sortWith (fun x y -> compare (f y) (f x))
let f x = x |> g ...

Context

StackExchange Code Review Q#43831, answer score: 6

Revisions (0)

No revisions yet.