I am a C# developer learning F#. Would appreciate any suggestions on how this code can be improved. Main points I am interested in:

  • Functional style of the code, absense of imperative style(C#) artifacts
  • F# collection data types and functions usage
  • General code(not algorithm) efficiency
  • Code style, naming etc.

    let life board =
        let flatten = Seq.cast<int> >> Seq.toArray
        let neighboursCount (board: int[,]) x y =
            board.[y..y+2, x..x+2]
            |> flatten
            |> Array.mapi (fun i el -> (i <> 4, el))
            |> Array.filter fst |> Array.map snd
            |> Array.filter ((=) 1) |> Array.length
        let expandBoard board =
            let N = Array2D.length1 board
            array2D [|
                yield                        [| yield board.[N - 1, N - 1]; yield! board.[N - 1, *]; yield board.[N - 1, 0] |]
                for i in [0..N - 1] do yield [| yield board.[i,     N - 1]; yield! board.[i,     *]; yield board.[i,     0] |]
                yield                        [| yield board.[0,     N - 1]; yield! board.[0,     *]; yield board.[N - 1, 0] |]
        let getNextBoard board =
            let eBoard = expandBoard board
            board |> Array2D.mapi (fun i j elem ->
                match neighboursCount eBoard j i with
                | 2 -> elem
                | 3 -> 1
                | _ -> 0)
        let isGameOver board prevBoards =
            board |> flatten |> Array.forall ((=) 0) || List.contains board prevBoards
        let rec lifeRec board prevBoards =
            printfn "%A \n" board
            if isGameOver board prevBoards
            then ()
            else getNextBoard board |> lifeRec <| board::prevBoards
        lifeRec board []
    let N = 5
    let rnd = System.Random()
    Array2D.init N N (fun _ _ -> rnd.Next 2) |> life
  • 2
    \$\begingroup\$ Please keep only one version of the code unless you wish a comparative-review, otherwise it's confusing which one to review. \$\endgroup\$ – t3chb0t Nov 18 '18 at 16:54

Review for the updated version (because there are no answers yet, I think you can still remove the first version).

I really like your implementation. Especially the idea of expanding the board instead of calculating the opposide indices as i did). That makes the code much more readable / understandable. I also like the way you imlemented the neighboursCount function - the combination of mapi, filter and map is a good example for functional programming.

In fact, I don't see much room for improvement.

Maybe you could rename neighboursCount into countNeighbours an move the board to the end of the argument list so it can be used like: eBorad |> countNeighbours j i

Within the isGameOver function, I think I would prefer writing prevBoards |> List.contains board over List.contains board prevBoards.

Thanks for sharing your solution :).

  • \$\begingroup\$ Thank you for the answer. \$\endgroup\$ – Andrew Boklashko Dec 13 '18 at 10:23
  • \$\begingroup\$ I believe neighboursCount came from my OO background, where you name object properties like that. Also that is a bit unusual that I can use piping where it seems to be unneded, but in fact version with piping is much more readable, so I accept your point. \$\endgroup\$ – Andrew Boklashko Dec 13 '18 at 10:29

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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