7
\$\begingroup\$

I started playing around recently with F# and I find it quite elegant and succinct language.

A common problem I like to solve in every language I start to learn is the coin flip problem, with the code below being the solution to this particular problem. What the code does is to flip a coin N times and perform M tests for each flip series. Finally, it prints in console a binomial distribution visual.

I am new to F# and I might being hasty here, I probably will learn this myself much later, but is anybody that can suggest a refactoring? Is it possible to make this even more succinct?

I solved the problem with code very similar to imperative style and I refactored my way to the following code, trying keep it as much as declarative as I could.

open System

let heads (random: Random) =
    random.Next(0, 2) = 1

let add x y = x + y

let rec calculateHeads n random =
    match n with
    | 0 -> 0
    | _ -> 
        let result = calculateHeads (n - 1) random
        let addToResult v = add v result
        match heads (random) with
        | true -> addToResult 1
        | false -> addToResult 0

let rec fill frequency n m random =
    match m with
    | v when v < 0 -> frequency
    | _ ->
        let count = calculateHeads n random
        Array.set frequency count (frequency.[count] + 1)
        fill frequency n (m - 1) random

let getFrequencies n m =
    let random = new Random()
    let frequency = Array.create (n + 1) 0
    fill frequency n m random

let rec displayAsterisk length =
    match length with
    | v when v = 1 -> "*"
    | v when v < 1 -> ""
    | _ -> "*" + (displayAsterisk (length - 10))

let rec displayRecursive list =
    match list with
    | [] -> ()
    | head::tail ->
        displayRecursive tail
        printfn "%d%s -- (%d)" head (displayAsterisk (head)) head

let displayCoin (frequency:int[]) =
    displayRecursive (frequency |> Array.toList)

[<EntryPoint>]
let main argv = 
    let frequency = getFrequencies 32 1000
    displayCoin frequency
    0 
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

I had some smaller points to make on several aspects of your code but I won't mention them since the whole thing can be written in more idiomatic and simpler style without using any recursion or mutation.

Note the use of Seq.sumBy and Array.countBy to cut down on a lot of the work:

let random = System.Random()

let heads () = random.Next(0, 2) = 1

let calculateHeads n =
    { 1 .. n } |> Seq.sumBy (fun _ -> if heads () then 1 else 0)

let getFrequencies n m =
    let counts =
        Array.init m (fun _ -> calculateHeads n)
        |> Array.countBy id
        |> Map
    Array.init (n + 1) (fun i -> counts.TryFind i |> Option.defaultValue 0)

let displayCoin frequency =
    for c in frequency do
        printfn "%d%s -- (%d)" c (String.replicate (c / 10) "*") c

let frequency = getFrequencies 32 1000
displayCoin frequency

I also made random a module level value instead of passing it around everywhere. The downside of this is you can't test individual functions deterministically.

If you're not already doing so, you should send code to F# interactive in your IDE to get a much faster feedback cycle for trying out ideas and exploring data.

\$\endgroup\$
1
  • \$\begingroup\$ Just a minor fix, on displayCoin method, it should print the current index in array, so it'd look like this: let displayCoin (frequency:int[]) = for i = 0 to frequency.Length - 1 do let value = frequency.[i] printfn "%d%s -- (%d)" i (String.replicate (value / 10) "*") value Great answer nonetheless, simplifies my code greatly! \$\endgroup\$ Commented May 27, 2018 at 22:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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