FizzBuzz with more parameters in Swift

Question

Here's my approach to an extensible version of the FizzBuzz challenge in Swift (so one can add more numbers to be checked against other than just 3 and 5).

I initially tried using dictionaries but since they don't necessarily preserve the order of elements it could lead to BuzzFizz return values.

I personally think that, while the program does work as expected, it isn't clear how it works. I mean, it's not as readable as I'd like it to be for doing such a simple task.

How can I improve it?

func fizzBuzz (n: Int, responsesByMultiples: [(Int, String)] = [(3, "Fizz"), (5, "Buzz")]) -> String {
    var result: String = ""

    for key in responsesByMultiples {
        let (multiple, response) = key,
            isMultiple: Bool = (n % multiple) == 0

        if isMultiple {
            result += response
        }
    }
    return result == "" ? String(n) : result
}
for i in 1...100 {
    print(fizzBuzz(n: i))
}

Answer 1

🔆 Bright side

Your code gives the correct output and delivers on the premise of extensibility.

📝 Suggestions

Here are some suggestions:

1. The name key is a name that doesn't tell a lot about the nature of the tuple. If you feel that it is appropriate, that would conflict with responsesByMultiples. Then It would have made more sense for the latter to be named keys. Personally, I'd prefer to call responses or rules.

2. These rules expressed by a tuple without labels is a little bit confusing. Better use a struct, or just add labels.

3. You can easily decompose a tuple this way:

   for (multiple, response) in responsesByMultiples {
       ...
   }
   

4. Calling the first element multiple is a little bit too optimistic (or pessimistic depending on the way you to see things), it presumes that there is a high probability that n is actually a multiple of multiple. It would make more sense to me to name the first element of the tuples dividsor.

5. n is a bit too generic, use dividend instead.

6. You don't have to specify the type in isMultiple: Bool, type inference can do the job for you. Generally speaking, specifying the type can help in reducing the compile times, but in this case, it wouldn't make a difference.

7. Instead of using (n % multiple) == 0, there is a nice syntax in Swift 5 :

   n.isMultiple(of: multiple)
   

   (this syntax exacerbates the problem mentioned in 4.)

8. To check that a String is empty, it is more efficient to check the .isEmpty property. Here is a benchmark that confirms it:

   import Foundation
   
   let array = (0..<1_000_000).map { _ in
       Double.random(in: 0..<1) < 0.8 ? "" : String("Hello".shuffled())
   }
   
   do {
       let start = Date()
       let result = array.filter { $0 == "" }
       let end = Date()
   
       print(result.count, end.timeIntervalSince(start))
   }
   
   do {
       let start = Date()
       let result = array.filter { $0.isEmpty }
       let end = Date()
   
       print(result.count, end.timeIntervalSince(start))
   }
   

   The execution times are respectively 44ms and 34ms.

💔 Putting it all together ❤️

Here a version of your code that takes the previous points into account :

func fizzBuzz (
    number dividend: Int,
    rules: [(Int, String)] = [(3, "Fizz"), (5, "Buzz")]
    ) -> String {

    var result: String = ""

    for (divider, response) in rules
        where dividend.isMultiple(of: divider) {
        result += response
    }

    return result.isEmpty ? String(dividend) : result
}

🧐 Further reading

You can find many implementations of this classic question on Code Review. Here is a quite informative one.