current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite
1

This code solves the problem of FizzBuzz. Is it possible in any way to improve it?

main = main' 1 where
main' n = do
    (putStrLn . choose) (show n, "Fizz", "Buzz", "FizzBuzz", n)
    if n < 100 then main' (succ n) else putStrLn "End!" 
    where
        choose (n0, n3, n5, n15, n) 
            | mod n 3 == 0 && mod n 5 == 0 = n15
            | mod n 5 == 0 = n5
            | mod n 3 == 0 = n3
            | True = n0
share|improve this question
    
Which definition of FizzBuzz are you using? Based on the first Google result the #s are fixed at [1..100], so there shouldn't be any inputs. –  jnewman Nov 24 '12 at 19:33

3 Answers 3

up vote 6 down vote accepted

You could separate your I/O from the pure code:

fizzBuzz :: Int -> String
fizzBuzz n | mod n 3 == 0 && mod n 5 == 0 = "FizzBuzz"
           | mod n 5 == 0                 = "Buzz"
           | mod n 3 == 0                 = "Fizz"
           | otherwise                    = show n

main = mapM print (map fizzBuzz [0..100])
share|improve this answer
    
Hm. This is really good example. Thank you. –  True Nov 21 '12 at 23:26
4  
I think mapM (print . fizzBuzz) [0..100] would be better. –  Anton Golov Nov 24 '12 at 21:08
3  
mapM_ would be even better. –  hammar Nov 25 '12 at 13:44
up vote 4 down vote

jaket is definitely right, the pure/impure distinction is important. My addition would be that you should avoid recomputing the modulo:

fizzBuzz :: Int -> String
fizzBuzz n | fizz && buzz = "FizzBuzz"
           | buzz         = "Buzz"
           | fizz         = "Fizz"
           | otherwise    = show n
           where fizz = mod n 3 == 0
                 buzz = mod n 5 == 0

sfb = map fizzBuzz [1..15]
share|improve this answer
    
Thanks! This is good instance. –  True Nov 23 '12 at 7:00
up vote 2 down vote

I had written something similar to Will, but the spec I read here says, that FizzBuzz should always cover [1..100], so my implementation was a bit different:

show' :: Int -> String
show' n
    | fizz && buzz = "FizzBuzz"    
    | buzz         = "Buzz"
    | fizz         = "Fizz"
    | otherwise    = show n
    where fizz = mod n 3 == 0
          buzz = mod n 5 == 0

fizzBuzz = [show' x | x <- [1..100]]

Will's idea about using where to cache the mod result was a nice idea IMHO.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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