I got me some feedback! Who knew, after 11 posts, people starting reading my posts and tweeting me feedback and ideas.

@Felienne in F# can you do ` List.sortBy (helper.getRank) hand
` instead of `List.sortBy (fun x-> helper.getRank(x)) hand`?

— Daan van Berkel (@daan_van_berkel) March 6, 2016

Turns out, you can! That makes my code a bit shorter.

Furthermore, I got a whole refactored version!

@Felienne I refactored the last part of your code, take a look https://t.co/brNoZm9VkP

— 🥑 vaskir (@kot_2010) March 6, 2016

let sortAndRev = List.sortBy helper.getRank >> List.rev
let findSuits x = List.filter (helper.getSuit >> (=) x)

let nextCard { Direction = direction; Hand = h} (history: 'a list list) (trump: Desi.Suit) : Desi.Card = 
    match h, history with
    | Desi.Hand [], _ -> ??? // what to do in this case?
    | _, [] -> ???           // what to do in this case?
    | Desi.Hand hand, thisTrick :: _ ->
       let orderedCards = sortAndRev hand
    
       match thisTrick, orderedCards with
       | [], h :: _ -> h
       | openCard :: _, _ -> 
    
        //first: can we follow suit?
        let openSuit = helper.getSuit openCard 
        let openRank = helper.getRank openCard
        let allOpenSuitinOrder = orderedCards |> findSuits |> sortAndRev
    
        match allOpenSuitinOrder with
        | h :: _ -> if helper.getRank h > openRank then h else h // it seems there is a bug here in logic?
        | [] -> 
           //if we have a trump, we will play it
           match hand |> findSuits |> sortAndRev with
           | h :: _ -> h
           | [] -> List.nth (sortAndRev hand) 0

Thank you kot_2010! It was not entirely correct*, but there are lots of good suggestions in it.

Before I could determine whether this was actually a correct refactoring, I of course first had to write some tests. In doing so I found an issue with my implementation!

let allOpenSuitinOrder = List.filter (fun x -> helper.getSuit(x) = openSuit) thisTrick

Here I filter the suit from the trick rather than from my hand, that should of course be:

let allOpenSuitinOrder = List.filter (fun x -> helper.getSuit(x) = openSuit) orderedCards

Also, I try get the trick before knowing the history is not empty. In kot’s version above I fixed this.

So, his improvements, let me go over them one by one:

Introducing small little helper functions

let sortAndRev = List.sortBy helper.getRank >> List.rev
let findSuits x = List.filter (helper.getSuit >> (=) x)

I like both suggestions, but I will name them a bit differently. I’ll name sortAndRev just sort as the Rev is more of an artefact of the coding of the numbers for ranks then something we want to stress.

Also, I think findSuit is better, even though you find multiple cards, you are looking for only one suit.

Applying this to my code gives:

   let nextCard {Direction = direction; Hand = h} (history : List<List>) (trump: Desi.Suit) : Desi.Card = 
      match h with
      | Desi.Hand hand ->
         
         let orderedCards = sortCardsbyRank hand
         let thisTrick = List.nth history 0

         match history with
            | [] -> List.nth orderedCards 0 //nothing the in history yet, we are the starting player, and we play our highest card.
            | _ -> 

               match thisTrick with
                  | openCard :: t -> 

                  //first: can we follow suit?
                  let openSuit = openCard |> helper.getSuit
                  let openRank = openCard |> helper.getRank

                  let allOpenSuitinOrder = orderedCards |> findSuit openSuit 

                  match allOpenSuitinOrder with
                     | h::t -> if helper.getRank(h) > openRank then h else List.nth (allOpenSuitinOrder|> List.rev) 0
                     | [] -> //no can do? Let's check trumps!

                     let allTrumpsinOrder = hand |> findSuit trump
      
                     //if we have a trump, we will play it
                     match allTrumpsinOrder with
                        | h::t -> h
                        | [] -> List.nth orderedCards 0 //otherwise we will play the highest card we have

Here you can spot the little mistake kot made, he wrote orderedCards |> findSuit instead of
orderedCards |> findSuit openSuit.

Also, I dropped the |> sortCardsbyRank because the filter preserve the sorting, so that is more efficient.

Getting thisTrick with pattern matching

I used a list nth to get the first element:

let thisTrick = List.nth history 0

but of course that can be pattern match with

	 match history with 
	 | thisTrick :: _

Good catch!

Doing two matches at once

The third suggestion is to combine two matches, so

    match h, history with
    | Desi.Hand [], _ -> ??? // what to do in this case?
    | _, [] -> ???           // what to do in this case?
    | Desi.Hand hand, thisTrick :: _ ->
       let orderedCards = sortAndRev hand

instead of

      match h with
      | Desi.Hand hand ->
         
         let orderedCards = sortCardsbyRank hand
         let thisTrick = List.nth history 0

         match history with
            | [] -> List.nth orderedCards 0 //nothing the in history yet, we are the starting player, and we play our highest card.
			
			etc....
	

That surely make the code a lot less indented shorter.

Applying both suggestions to my code gives:

   let nextCard {Direction = direction; Hand = h} (history : List<List>) (trump: Desi.Suit) : Desi.Card = 
      match h, history with
      | Desi.Hand hand, [] -> 
          let orderedCards = sortCardsbyRank hand
          List.nth orderedCards 0
        
      | Desi.Hand hand, thisTrick :: _ ->

         match thisTrick with
            | openCard :: t -> 

            let orderedCards = sortCardsbyRank hand
            //first: can we follow suit?
            let openSuit = openCard |> helper.getSuit
            let openRank = openCard |> helper.getRank

            let allOpenSuitinOrder = orderedCards |> findSuit openSuit |> sortCardsbyRank

            match allOpenSuitinOrder with
               | h::t -> if helper.getRank(h) > openRank then h else List.nth (allOpenSuitinOrder|> List.rev) 0
               | [] -> //no can do? Let's check trumps!

               let allTrumpsinOrder = hand |> findSuit trump
      
               //if we have a trump, we will play it
               match allTrumpsinOrder with
                  | h::t -> h
                  | [] -> List.nth orderedCards 0 //otherwise we will play the highest card we have

As you can see, there is a little price, because for both options for history (empty and non empty) we need to order the cards. I think kot missed the case where history is empty.

So it is nice, but it comes I at the costs of a bit repetition and, also it might be harder to read the code because you have to read options of unrelated things together (that is a hunch though)

But granted, I had not even considered this option, so I am grateful for the suggestion!

Finally, there is no bug in the logic where Kot points it our, if we cannot go higher we reverse once more and play the lowest card (no use in trowing away an higher card) But granted, with no tests or comments on the line that is hard to understand.

* Fun exercise: try to spot the error before you read on