Domemo F#実装のリファクタリング

趣旨

友人がC# + DxライブラリでDomemoのGUI組んでるので、メインロジックはF#を組み込んで使ってもらおう！
これまでVBとかC#単体でしか使ってきてないので.NetFrameworkの利点を体感したい
ちょうどいい感じに分業っぽいし。

Domemo実装によるプログラミング言語比較　その１のF#コードを改変。

成果物

ソースコードはBitBucketにも入れてますので、MITライセンスの元でお好きにしてください!
git clone https://[e-mail]/aoi_erimiya/domemo.git

改善すべき問題点

• 標準入出力を使う処理をクラスの中に埋め込まない
• 所々に残念な感じの決め打ち実装が残っているので、いい感じに直す
• 他言語で実装したDomemoプログラムで得たフィードバックを反映する
• メイン処理が長いのでクラスに分離して、移植しやすいようにする
• F#特有の実装(Listの操作など)をクラスに隠ぺいして、C#で書き換えなくていいようにする
• 変数名などリファクタリングする(数年ぶりにリーダブルコード読み返して愕然)

変更点

１：Playerのやるべき事を整理

• guessCard()をメインロジックから切り出し
• show()→cardsToString()に改めて、文字列返却に
• matchCard()はマッチしたかどうかを返却するように変更
  　※他言語実装のフィードバック

domemo.fs

type Player(name:string, cards:List<int>) = 
    let name:string = name
    let mutable cards:List<int> = cards

    member x.Name with get() = name

    member x.cardsToString(isMask:bool) = 
        let mutable cardsStr = ""
        if isMask then
            cards |> List.iter (fun x -> cardsStr <- cardsStr + "X")
        else
            cards |> List.iter (fun card -> cardsStr <- cardsStr + card.ToString())
        cardsStr

    member x.checkCard() = cards.Length = 0

    member x.guessCard(rand: Random) =
        rand.Next(1, 8)

    member x.matchCard(guessCard:int) = 
        if List.contains guessCard cards
        then
            let idx = List.findIndex (fun card -> card = guessCard) cards
            cards <- List.concat [ cards.[0..idx-1]; cards.[idx+1..cards.Length-1] ]
            true
        else
            false

２：GameMasterクラスの追加

• 場のカードの管理を任せる
• カードシャッフル処理を高速化
  　※F#におけるシャッフル処理の実装のフィードバック

domemo.fs

type GameMaster() =
    let mutable cards:List<int> = []
    let mutable openCards:List<int> = []

    member x.Cards with get() = cards
    member x.OpenCards with get() = openCards

    member x.addOpenCards(addCard:int) =
        openCards <- List.sort <| List.append [addCard] openCards

    member x.createShuffledCards(rand:Random) = 
        let mutable cards = []
        for i in 1..7 do
            for _ in 1..i do
                cards <- List.append [i] cards

        let swap (array: _[]) x y =
            let tmp = array.[x]
            array.[x] <- array.[y]
            array.[y] <- tmp

        // shuffle an array (in-place)
        let shuffle array =
            Array.iteri (fun i _ -> swap array i (rand.Next(i, Array.length array))) array

        let cardArray = Array.ofList cards
        shuffle cardArray
    
        Array.toList cardArray
    
    // To run before playing
    member x.preparationGame(rand:Random) = 
        cards <- x.createShuffledCards(rand)
        x.takeInitialOpenCards(cards)

    member x.takeInitialOpenCards(cards:List<int>) =
        openCards <- List.sort cards.[cards.Length-4..cards.Length-1]

３：クラスに切り出せなかったロジックたち

• 標準出力するロジックはただの関数に据え置き
• プレイヤー作成をメインロジックから分離
• こっそりプレイヤー名を指定できるように改修

domemo.fs

let showCards(label:string, cards:List<int>) =
    printf "%s -> " label
    cards |> List.iter (fun card -> printf "%d" card)
    printfn ""

let showPlayers(players:List<Player>) = 
    players |> List.iter (fun player -> (Console.WriteLine("{0} -> {1}", player.Name, player.cardsToString(List.last players = player))))

let createPlayers(cards:List<int>, playerNames:List<string>) = 
    let mutable players = []
    for i in 0..3 do
        let player = Player(playerNames.[i], cards.[5*i..5*(i+1)-1])
        players <- List.append [player] players
    players

４：メインロジックの変更

• 可読性アップのために変数名など見直し
• 移植しやすくするため、標準入出力を使う処理をメインロジックに移動

domemo.fs

[<EntryPoint>]
let main argv = 
    let rand = Random()

    let gm = GameMaster()
    gm.preparationGame(rand)

    let players = createPlayers(gm.Cards, ["Mizuki";"Izumi";"Ryoko";"Aoi"])
    
    let mutable isGameOver = false
    
    let rec nextGameRound() =
        printfn "----------------------"
        printfn "*Field*"
        showCards("Open cards", gm.OpenCards)
        printfn ""
        printfn "*Players*"
        showPlayers(players)
        printfn "----------------------"
        
        Thread.Sleep(500)
        
        for player in players do

            if not isGameOver then
                Thread.Sleep(500)
            
                let guessCard =
                    if List.last players = player then
                        printf "Please input guess number! -> "
                        Console.ReadLine() |> int
                    else
                        player.guessCard(rand)

                printfn "%s called -> %d" player.Name guessCard

                if player.matchCard(guessCard) then            
                    printfn "jackpot!"
                    gm.addOpenCards(guessCard)
                else
                    printfn "miss!"

                if player.checkCard() then
                    printfn "----------------------"
                    printfn "%s is Win!" player.Name
                    isGameOver <- true
                else
                    printfn ""
        
        if isGameOver then
            ()
        else
            printfn "Next round...."
            nextGameRound()
    
    nextGameRound()

    System.Console.ReadLine() |> ignore
    0

やってみて

元々のコードよりもだいぶ見通しが良くなった
もうちょっと関数型っぽくやりたいけど、再利用性を考えると
オブジェクト指向に寄ってきて、なんかC#っぽいコードになっちゃう感じがする。

showPlayers()のConsole.WriteLine()は苦肉の策で、
本当はprintfn使いたかったけど今のF#力ではエラーを解決できず。
スペースで区切ってるし、タプルにしても直らないし…
一体何が悪いのだろう？

domemo.fs

let showPlayers(players:List<Player>) = 
    players |> List.iter (fun player -> (printfn "%s -> %s" player.Name player.cardsToString(List.last players = player)))

→2018/08/03 @ozwkさんに早速コメントいただきまして、即解決しました
　printfnの引数に関数の結果渡したいときは、()で囲む必要あったんですね。※コメント欄参照

domemo.fs

let showPlayers(players:List<Player>) = 
    players |> List.iter (fun player -> (printfn "%s -> %s" player.Name (player.cardsToString(List.last players = player))))

あとはお任せ

とまあ、色々あったけど、あとは友人にこのクラスを投げつけるだけ！
GUI化 + オンライン対戦機能(勝手な要望)の実装楽しみにしています

ご意見など

F#の専門家の方、もっと綺麗に書けるよとかご意見ありましたら
ぜひ教えてください