4
\$\begingroup\$

I have been adding and tinkering a bit with the code from my Baloot tally finder. I updated the code a bit and factored a new things, but for this post I am trying different stuff: to detect whether the cards in your hand have a "project" or not.

Baloot has some peculiar names for these projects, but they're similar to Poker hands. Since you only ever 7 cards in your hand at the beginning of a round, some shortcuts in the code were done this way.

  • A Sira is three in a row. Sometimes when you're lucky you have two of them in your hand. I am not sure how to deal with that.
  • A Fifty is four in a row
  • A Hundred is either five in a row, or four of a kind.
  • A FourHundred is four Aces.
  • A Baloot, which the game is named after, is King and Queen of the trump suit.

I am a bit unsure about the whole function composition thing, as I feel I took it a bit too far. If the code could somehow be made simpler and/or more readable, I am all ears. Ideas for better names for the functions would be much appreciated as well.

Without further ado:

Domain Model
// Similar to previous post but edited a bit.
// Ranks 2 - 6 don't exist in Baloot, so (A, 2, 3) sequence isn't a thing.

type Rank = Ace | King | Queen | Jack | Ten | Nine | Eight | Seven
type Suit = Hearts | Clubs | Diamonds | Spades

type Card = 
    { Rank : Rank
      Suit : Suit }

    member this.IsPicture = // These two properties are instead of simple functions to simplify the code and make it more reusable.
        match this.Rank with
        | Nine | Eight | Seven -> false
        | _ -> true

    member this.SortValue = 
        match this.Rank with
        | Ace -> 0
        | King -> 1
        | Queen -> 2
        | Jack -> 3
        | Ten -> 4
        | Nine -> 5
        | Eight -> 6
        | Seven -> 7

type Mode = Sun | Trump of Suit 

type Hand = 
    { Mode : Mode
      Cards : Card list }
Helper functions
// only works for lists of distinct integers. 
// This function is just to make the logic of the next one clearer.
let hasIntSeqOf n = 
    List.sort
    >> List.mapi (-) // subtract number from index so consecutive numbers have the same value.
    >> List.groupBy id // group by value
    >> List.filter(snd // If any group has (n <= count) members, a sequence exists
                   >> List.length
                   >> (<=) n)
    >> List.map Some

let cardsHaveSeqOf n = // Sig: int -> Card list -> Card list option list
    List.sortBy (fun (c : Card) -> c.SortValue)
    >> List.mapi (fun i c -> (i - c.SortValue, c)) // tuple with the card and the invariant if there are consecutive cards 
    >> List.groupBy fst //group by the invariant
    >> List.map snd // then strip it out
    >> List.filter (List.length >> (<=) n)
    >> List.map ((List.map snd) >> Some) // clean up



let hasSeqOf n hand = // Sig: int -> Hand -> Card list option list. I am not sure this is a good idea.
    hand.Cards
    |> List.groupBy (fun c -> c.Suit)
    |> List.map (snd >> cardsHaveSeqOf n)
    |> List.concat // This is oddish.
    |> List.filter ((<>) None)

let cardsHaveFourOfAKind = 
    List.groupBy (fun c -> c.Rank)
    >> List.map snd
    >> List.tryFind (List.length >> (=) 4)
Actual functions
// All these functions return None if nothing is found and Some(Card list) 
// if a project is found, including the cards that make up the project.
// Somehow I imagine these functions would be on a function list of some 
// sort that I can use to quickly analyze hands and extract that info. I am 
// not there yet.

let hasOneSira = 
    hasSeqOf 3 >> function 
    | Some(cl) :: [] -> Some(cl)
    | _ -> None

let hasTwoSira = // I would rather not have this function but did not know how to deal with this specific circumstance.
    hasSeqOf 3 >> function 
    | Some(cl1) :: Some(cl2) :: _ -> Some(cl1 @ cl2)
    | _ -> None

let hasFifty = 
    hasSeqOf 4 >> function // there can be only one (four in a row)
    | Some(cl) :: _ -> Some(cl)
    | _ -> None

// Two separate functions for Hundred because sometimes they exist in the 
// same hand and share a card , but the card can be only in one project.    
let hasHundredOne = 
    hasSeqOf 5 >> function // there can be only one (five in a row)
    | Some(cl) :: _ -> Some(cl)
    | _ -> None

let hasHundredTwo hand = 
    hand.Cards
    |> cardsHaveFourOfAKind
    |> function 
    | Some(c :: tl) when c.IsPicture && c.Rank <> Ace -> Some(c :: tl)
    | _ -> None

let hasFourHundred hand = 
    hand.Cards
    |> cardsHaveFourOfAKind
    |> function 
    | Some(c :: tl) when c.Rank = Ace -> Some(c :: tl)
    | _ -> None

// This function is the only reason for the Hand type, instead of simply a Card list.
let hasBaloot hand = 
    match hand.Mode with
    | Sun -> None
    | Trump(t) -> 
        let king = List.tryFind (fun c -> c.Rank = King && c.Suit = t) hand.Cards
        let queen = List.tryFind (fun c -> c.Rank = Queen && c.Suit = t) hand.Cards
        match king, queen with
        | Some(k), Some(q) -> Some([ k; q ])
        | _ -> None

Any comments on :

  • Readability
  • Refactoring oppurtunities
  • Shortening code while making it more coherent
  • Function naming
  • the algorithms used, even if primitive.

are appreciated.

Edit: I fixed a bug from the original code if two Siras existed from the same suit. It led to some oddish code.

\$\endgroup\$
  • \$\begingroup\$ can you make the Q self-contained? I didn't want to edit the types as not to mess it up. \$\endgroup\$ – s952163 Jul 28 '16 at 10:45
  • \$\begingroup\$ @s952163 Sure. Will add the relevant types in. \$\endgroup\$ – asibahi Jul 28 '16 at 14:03
  • \$\begingroup\$ Hello! Please don't make changes to the original post once it has been reviewed, as that invalidates the current answers. Please see our meta side on performing iterative reviews for more information! \$\endgroup\$ – syb0rg Jul 28 '16 at 14:17
  • \$\begingroup\$ @syb0rg I didn't edit any of the algorithms or functions I just clarified the types that I was already using . \$\endgroup\$ – asibahi Jul 28 '16 at 14:19
  • \$\begingroup\$ @syb0rg I can add them as a PS if that's fine with you. \$\endgroup\$ – asibahi Jul 28 '16 at 14:20
3
\$\begingroup\$

This looks quite interesting. I'm surprised you haven't received more comments/answers. I would need some more time to go through the code. So below are some observations, will edit the answer once I digested all of it.

I think you can just use |> where you use function composition. Although in some places it does make sense, like List.map (snd >> cardsHaveSeqOf n). I tend to use >> for creating new functions that are more readable.

In hasSeqOf instead of |> List.filter ((<>) None) you could just say |> List.choose id.

I find the pattern matching with function difficult to read when you're piping into it. Maybe you can put on the same line, and indent or just stick to match.

I actually liked when in the previous Q you defined type Mode = Sun | Trump of Suit on one line.

I think C# people tend to use this., it could be anything though, if you don't want to trip up the linter you could use __. (double underscore).

Add 160729: On the hasIntOfSeq function. I redid it with |>. Is my understanding correct that this is what you're trying to extract? Initially I thought about using pairwise to get the consecutive numbers but found the List.mapi idea cool.

let xs = [10;9;12;11;21;22;23;30;40;50]
let mapmap2 x = (List.map >> List.map) snd x
let map2 x = List.map snd x

let isIntSeqOf xs =
    xs |> List.sort 
       |> List.mapi (fun i x -> (i - x,x))
       |> List.groupBy fst |> map2 |> mapmap2 
       |> List.filter (fun x -> x.Length >=3)

isIntSeqOf  xs
// val it : int list list = [[9; 10; 11; 12]; [21; 22; 23]]

After taking another look, I'm more convinced that you're better off with using |> except for some helper function. This way you can build up the larger function by piping into smaller functions, and test out the result as you go along. I redid your cardsHaveSeqOf function and merged it with hasSeqOf. I'm not saying it's pretty... I'm also simplifying a bit as it is probably better to reincorporate Some. This will return a Card list list, so it can return two Siras, one in each sub-list.

We better use >> somewhere!

let mapmap2 x = (List.map >> List.map) snd x
let map2 x = List.map snd x 

let cardsHaveSeqOf n (hand:Hand) = 
    hand.Cards |> List.groupBy (fun c -> c.Suit)         
               |> map2 
               |> List.map (List.sortBy (fun c -> c.SortValue))
               |> List.map (List.mapi (fun i c -> (c.SortValue - i, c))) 
               |> List.map (List.groupBy fst) |>  mapmap2  
               |> List.collect mapmap2
               |> List.filter (fun x -> x.Length = n)

Now for the pattern matching (again I'm simplifying a bit), I think the function with >> was a bit of an overkill. You can just simply do match:

let hasTwoSira (cl:Card list list ) =
    match cl with 
    |  [cl1;cl2] when cl1.Length =3 && cl1.Length =3  -> cl1 @ cl2 
    | _ -> [] 

let hasOneSira (cl:Card list list) =
    match cl with
    | [cl1] when cl1.Length = 3 -> cl1
    | _ -> []

If you go down this route, it has the advantage of putting together a quick hand-checker list of functions:

[hasTwoSira;hasOneSira] |> List.map (fun x -> x sira2)

where sira2 is some hand.

There is another way to do pattern matching, using Active Patterns. Active Patterns can hide the complex implementation of the pattern matching. For example:

let (|OneSira|TwoSira|Fifty|Other|) ((n:int), (hand:Hand)) = 
        let result = cardsHaveSeqOf n hand
        match result with 
        | [cl1] when cl1.Length = 3 -> OneSira
        | [cl1;cl2] when cl1.Length = 3 && cl2.Length =3 -> TwoSira
        | [cl1]  when cl1.Length = 4 -> Fifty
        | _ ->  Other

And voila we can do some magic with the hand:

let checkHand (n:int) (hand:Hand) =
    match (n,hand) with
    | OneSira x -> "One"
    | TwoSira x -> "Two"
    | Fifty x ->  "Fifty"
    | _ -> "Other"

Here's some test data:

let sun =  Sun
let card1 =  {Rank=Ace;Suit=Hearts}
let card2  = {Rank=King;Suit=Hearts}
let card3  = {Rank=Queen;Suit=Hearts}
let card4 = {Rank=Jack;Suit=Diamonds}
let card5 = {Rank=Ten;Suit=Diamonds}
let card6 = {Rank=Nine;Suit=Diamonds}
let card7  = {Rank=Seven;Suit=Clubs}
let card8  = {Rank=Nine;Suit=Spades}
let hand =  {Mode=sun; Cards=[card3;card1;card8;card7;card6;card2;card5;card4]}

let card11 =  {Rank=Ace;Suit=Hearts}
let card12  = {Rank=King;Suit=Hearts}
let card13  = {Rank=Queen;Suit=Hearts}
let card14 = {Rank=Jack;Suit=Hearts}
let card15 = {Rank=Ten;Suit=Diamonds}
let card16 = {Rank=Nine;Suit=Diamonds}
let card17  = {Rank=Seven;Suit=Clubs}
let card18  = {Rank=Nine;Suit=Spades}
let hand2 =  {Mode=sun; Cards=[card13;card11;card18;card17;card16;card12;card15;card14]}

let sira2 = cardsHaveSeqOf 3 hand
let fifty = cardsHaveSeqOf 4 hand2 

And let's try it out:

[hasTwoSira;hasOneSira] |> List.map (fun x -> x sira2)
[hasTwoSira;hasOneSira] |> List.map (fun x -> x fifty)

You could I guess apply a list of functions to a list of hands. And using the checkHand function:

checkHand 3 hand //val it : string = "Two" 
checkHand 2 hand //val it : string = "Other"
checkHand 4 hand2 //val it : string = "Fifty"

Now this is obviously buggy and incomplete but I hope by not too much!

\$\endgroup\$
  • \$\begingroup\$ Yes this is pretty much what I'm trying to go for. I will play with some of these see how far it gets me. \$\endgroup\$ – asibahi Jul 29 '16 at 15:07
  • \$\begingroup\$ Great. Let me know if you have any questions. But to be frank your code is quite good, and a lot of the things at this level have to do with personal preference. Regarding the active patterns probably you should use parametrized or partial APs in this case, if you go down that route. \$\endgroup\$ – s952163 Jul 30 '16 at 0:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.