3
\$\begingroup\$

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]
\$\endgroup\$
3
  • \$\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
    Commented Sep 10, 2015 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\$ Commented Sep 11, 2015 at 0:01
  • \$\begingroup\$ OK, have added a pointer to how more efficient implementations might be considered. \$\endgroup\$
    – itsbruce
    Commented Sep 11, 2015 at 0:44

2 Answers 2

2
\$\begingroup\$

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]
\$\endgroup\$
7
  • 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\$ Commented Sep 10, 2015 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
    Commented Sep 10, 2015 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\$ Commented Sep 10, 2015 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
    Commented Sep 10, 2015 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
    Commented Sep 10, 2015 at 21:41
4
\$\begingroup\$

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

\$\endgroup\$
6
  • \$\begingroup\$ Very very interesting. I suppose I've been tainted by seeing the generic implementation to think outside the box! \$\endgroup\$ Commented Sep 11, 2015 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
    Commented Sep 14, 2015 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
    Commented Sep 14, 2015 at 8:35
  • \$\begingroup\$ What you mention your second comment is precisely what I would do to generalise this, yes. \$\endgroup\$
    – itsbruce
    Commented Sep 14, 2015 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
    Commented Sep 14, 2015 at 21:23

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.