10
\$\begingroup\$

So I made a quick programme in F# to do FizzBuzz, I'm trying to learn more languages, so I figured I'd do something functional for a change. (Haskell is also on the list.)

FizzBuzz is pretty self-explanatory: every third value print Fizz, every fifth print Buzz (possible both if it's a multiple of 3 and 5).

[<EntryPoint>]
let main argv = 
    let fizzBuzzSequence = seq {
        for value in 1..100 do
            let valueMod3 = value % 3
            let valueMod5 = value % 5

            if valueMod3 = 0 && valueMod5 = 0 then yield "FizzBuzz"
            else if valueMod3 = 0 && valueMod5 <> 0 then yield "Fizz"
            else if valueMod3 <> 0 && valueMod5 = 0 then yield "Buzz"
            else yield value.ToString()
    }
    for value in fizzBuzzSequence do printfn "%s" value
    0
\$\endgroup\$
6
\$\begingroup\$

First off, I don't see a real need for these two variables:

let valueMod3 = value % 3
let valueMod5 = value % 5

To me, they just seem a little unnecessary. In fact, writing out the expression value % 3 or value % 5 takes the exact same amount of keystrokes as writing valueMod3 or valueMod5.

Next, I'd highly recommend that you get into the habit of using Match Expressions rather than plain old if statements, as it's more functional, and will give you more benefits, like Pattern matching, over if statements. This means that this small chunk of if statements:

if valueMod3 = 0 && valueMod5 = 0 then yield "FizzBuzz"
else if valueMod3 = 0 && valueMod5 <> 0 then yield "Fizz"
else if valueMod3 <> 0 && valueMod5 = 0 then yield "Buzz"
else yield value.ToString()

Would become a Match Expression, like this:

match n with
| n when n % 3 = 0 && n % 5 = 0 -> yield "FizzBuzz"
| n when n % 3 = 0 -> yield "Fizz"
| n when n % 5 = 0 -> yield "Buzz"
| _ -> yield n.ToString()

After all these improvements to your code, I ended up with the following, working code:

let fizzbuzzSequence = seq { 
    for n in 1 .. 100 do
        match n with
        | n when n % 3 = 0 && n % 5 = 0 -> yield "FizzBuzz"
        | n when n % 3 = 0 -> yield "Fizz"
        | n when n % 5 = 0 -> yield "Buzz"
        | _ -> yield n.ToString()
}

[<EntryPoint>]
let main argv = 
    for item in fizzbuzzSequence do
        System.Console.WriteLine(item)

    0
\$\endgroup\$
  • \$\begingroup\$ Variables do not exist solely for saving keystrokes ... or even primarily for that purpose. \$\endgroup\$ – phoog Dec 5 '15 at 4:16
  • \$\begingroup\$ @phoog I'm aware of that. There's simply no point in creating those two variables though, so I pointed that out. \$\endgroup\$ – Ethan Bierlein Dec 5 '15 at 4:20
  • \$\begingroup\$ There could be a point, though likely not in this context: Division is expensive. The variables allow you to avoid repetitious division. \$\endgroup\$ – phoog Dec 5 '15 at 4:22
3
\$\begingroup\$

I have some general comments:

  • Why limit to a predefined sequence? – You could pass a range to your sequence generator, which would allow for you to generate a random sequence. Like in:

    let my_sequence range = seq { 
        for value in range do 
           ...
     }
    
  • Allow for other fizzbuzz words – Instead of hard coding the 3 & 5 (and the combination of 15), use a dictionary to allow for extending the choices. This could either be a mutable dictionary, or you could make it non-mutable dict like in:

    fizzbuzzWords = dict[3, "Fizz"; 5, "Buzz"; 7, "Boom"]
    
  • Concatenate parts to avoid repeating module operations – In your code you do % 3 and % 5, and the extraneous % 15. If you instead did the different modulos one by one, you could concatenate the parts together:

    fizzbuzzParts value |> String.concat ""
    
  • Move bits and pieces out of [<EntryPoint>] – Don't do everything in the main function, keep that succint and to the point.

I'll leave it to the reader to actually implement these suggestions! Why should the reader have all the fun? Here is a working example (see code at repl.it):

open System.Collections.Generic

// Non-mutable dict of fizzbuzz words
let fizzbuzzWords = dict[3, "Fizz"; 5, "Buzz"; 7, "Boom"]

// Generates single complete fizzbuzz word, or nothing if not a fizzbuzz number
let fizzbuzzParts number =  seq {
    for pair in fizzbuzzWords do
        if number % pair.Key = 0 then yield (pair.Value)
    }

// Generator for a sequence
let fizzbuzzSequence range = seq {
    for value in range do
        let fizzPart =  fizzbuzzParts value |> String.concat ""
        yield if fizzPart <> "" then fizzPart else value.ToString()
    }

[<EntryPoint>]
let main args = 
    for value in fizzbuzzSequence [ 104 .. 121 ] do
        printf "%s " value 

    printfn ""
    0

In stead of using a mutable Dictionary, I here use non-mutable dictionary, and some helper generators. This produces the following output:

104 FizzBuzzBoom 106 107 Fizz 109 Buzz Fizz Boom 113 Fizz Buzz 116 Fizz 118 Boom FizzBuzz 121

I wasn't entirely satisfied with this solution, even though I haven't programmed in before, and have been googling my way to all the bits and pieces to compile this answer! :-)

So I kept on googling, and landed on this alternate solution (see updated code for following and various other variants):

open System.Collections.Generic

let fizzbuzz number =
   seq {                          // Build sequence of word parts
        for pair in dict [3, "Fizz";
                          5, "Buzz";
                          7, "Boom"] do
            if number % pair.Key = 0 then  // If modulo is 0 ...
                yield pair.Value           // ... yield a word part
    }                             
    |> String.concat ""           // Concatenate the word parts
    |> function                   // Do a match on the word
       | "" -> number.ToString()  //   If nothing, use number
       | word -> word             //   else use the word


[<EntryPoint>]
let main args = 

    { 104 .. 121 }
    |> Seq.iter (fizzbuzz >> printf "%s ")

    printfn ""
    0

The output is the same, but this version is somewhat cleaner and a little more functional. This solution focuses more on getting a single fizzbuzzWord based on an inline list of words.

\$\endgroup\$

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.