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]
  • \$\begingroup\$ Do you want to tweak this implementation or are you looking for a "better" implementation? If so, how do you define better? Readability? Efficiency? Idiom? \$\endgroup\$ – itsbruce Sep 10 '15 at 23:45
  • \$\begingroup\$ @itsbruce Mainly compactness and efficiency, I suppose "better" IS very subjective but I'm open to any kinds of improvements, everything is worth consideration! \$\endgroup\$ – Tristan McPherson Sep 11 '15 at 0:01
  • \$\begingroup\$ OK, have added a pointer to how more efficient implementations might be considered. \$\endgroup\$ – itsbruce Sep 11 '15 at 0:44

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

Instead of:

mod x y

for readability.

main can be written to avoid all those quotes making output nicer:

main :: IO()
main = putStrLn $ unlines $ map toFizz [1..100]
  • 1
    \$\begingroup\$ In my original code, the case statement caches the mod x 3 and mod x 5 and make it so that it does not have to check mod x 15 also, so I would argue that it's the better of the two. \$\endgroup\$ – Tristan McPherson Sep 10 '15 at 20:46
  • \$\begingroup\$ @TristanMcPherson thanks for commeting. Beware of premature optimization. A program to print 100 numbers needs not to be fast. \$\endgroup\$ – Caridorc Sep 10 '15 at 21:00
  • \$\begingroup\$ @Caridorc of course not, but the question was for the BEST fizzbuzz ;), even if it's just fizzbuzz, also why is it something to beware of? \$\endgroup\$ – Tristan McPherson Sep 10 '15 at 21:15
  • \$\begingroup\$ @TristanMcPherson this is a very big concept, no way to discuss it all in comments here. It is just that you should always balance readability and optimizations, and it is not always easy to do so. \$\endgroup\$ – Caridorc Sep 10 '15 at 21:23
  • 1
    \$\begingroup\$ This version is quadratic in the number of primes you consider (and you have to repeat yourself). See e.g. this thread for linear versions. \$\endgroup\$ – gallais Sep 10 '15 at 21:41

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 unfoldr

  • \$\begingroup\$ Very very interesting. I suppose I've been tainted by seeing the generic implementation to think outside the box! \$\endgroup\$ – Tristan McPherson Sep 11 '15 at 1:19
  • \$\begingroup\$ I think you've misunderstood that comment by gallais. the code size is what's exploding (if we'd add more factors for it to handle); yours will too - in the hand-written list's length, which must contain lcm(all_factors) entries. you can remedy this by defining a list-generating code in the spirit of that reddit thread. the OP code's run time is certainly linear - it performs no more than 1 test for each factor (here there's just two of them), per each x. \$\endgroup\$ – Will Ness Sep 14 '15 at 8:34
  • \$\begingroup\$ (contd.) But the idea of precalculating a rolling wheel is indeed nice. You could even generate that list without doing any divisibility tests too, by generating and merging the lists of multiples for each factor, for a fully generative solution! This will offset the cost of initial tests with the cost of merging those separate lists into one. \$\endgroup\$ – Will Ness Sep 14 '15 at 8:35
  • \$\begingroup\$ What you mention your second comment is precisely what I would do to generalise this, yes. \$\endgroup\$ – itsbruce Sep 14 '15 at 8:37
  • 1
    \$\begingroup\$ okay, so taking your idea and running with it, I got fzbz = zipWith (flip ($)) [1..] . map (\s-> if null s then show else const s) . foldr1 (zipWith (++)) . map (\(d,s) -> tail . cycle $ s : replicate (d-1) "") $ zip [1,3,5] ["","fizz","buzz"]. an altogether nice result. \$\endgroup\$ – Will Ness Sep 14 '15 at 21:23

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.