Is there any way I can make this FizzBuzz better?
toFizz :: Int -> String toFizz x = case (mod x 3, mod x 5) of (0, 0) -> "FizzBuzz" (0, _) -> "Fizz" (_, 0) -> "Buzz" (_) -> show x main :: IO() main = mapM_ (print . toFizz) [1..100]
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It only takes a minute to sign up.Sign up to join this community
Sure, I can suggest two improvements:
One is rewriting toFizz in a more explicit manner:
toFizz :: Int -> String toFizz x | x `mod` 15 == 0 = "FizzBuzz" | x `mod` 3 == 0 = "Fizz" | x `mod` 5 == 0 = "Buzz" | otherwise = show x
Your pattern match with (mod 3, mod 5) puzzled me a fair bit. Also use
x `mod` y
mod x y
main can be written to avoid all those quotes making output nicer:
main :: IO() main = putStrLn $ unlines $ map toFizz [1..100]
As gallais has mentioned, the cost of your calculations increases as x increases.
I'm always struck by how few people, when faced with fizzBuzz or similar challenges (like finding the first
n numbers which are divisible by 3 and/or 5), realise there is no need to work backwards from x to the original primes. It's much more efficient to work forwards. After all, you know the starting primes, 3 and 5. One might consider, for example, generating a map where the keys are the numbers 1 to 100 (or x to y) and the value is
show. Then iterate over the multiples of 3 between x and y, replacing the value with
const "fizz" and so on. Then iterate over the keys of the map in order, applying the value (a function) to the key. There are smarter (and more efficient) ways to work forwards, but it illustrates the principle.
Here's a relatively naive forwards-iterating implementation of
toFizzBuzz I just thought up.
fizz :: Int -> String fizz = const "fizz" buzz :: Int -> String buzz = const "buzz" fizzbuzz :: Int -> String fizzbuzz = const "fizzbuzz" fizzbuzzFuncs = cycle [show, show, fizz, show, buzz, fizz, show, show, fizz, buzz, show, fizz, show, show, fizzbuzz] toFizzBuzz :: Int -> Int -> [String] toFizzBuzz start count = let offsetFuncs = drop (mod (start - 1) 15) fizzbuzzFuncs in take count $ zipWith ($) offsetFuncs [start..]
Note that it can work from any starting point (even negative n) for any range, within the limits imposed by
Int. Now, there are smarter ways to do this kind of thing (and certainly more idiomatic and generalised), but that example is still more efficient than 99% of the fizzBuzz attempts I see, Only does any arithmetic once.
Work forwards, not backwards. Don't go looking expensively for things you can easily generate.
Challenge: consider a solution using