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.
for (var i = 1; i <= 100; ++i) {
    var fizzBuzz = ""

    if i % 3 == 0 {
        fizzBuzz += "Fizz"
    }

    if i % 5 == 0 {
        fizzBuzz += "Buzz"
    }

    if fizzBuzz == "" {
        fizzBuzz += "\(i)"
    }

    println(fizzBuzz)
}

I don't really like comparing strings with ==, but apparently, that's how you do it in Swift (and there's not another option).

The parenthesis in if statements are optional in Swift. Should that be a thing, or should we stick with them? The curly-braces were optional in Objective-C (and lots of programming languages) but they're not in Swift. Despite their former optionality, I never thought it was a good idea to not use them--is it a good idea to not use parenthesis here?

Not explicitly declaring the type of variable is now a thing in Swift (although the variable still has an explicit type, it's just implicitly determined). Is it okay to let the type be implicitly determined, or should we stick with explicitly declaring the type?

share|improve this question
add comment

5 Answers 5

up vote 16 down vote accepted

Is this FizzBuzz Swift-y?

Kinda, but it could be a lot better. Here's what I would do to fix it:

  • Extrapolate this code into a method, then call the method from the for loop.

    func fizzbuzz(i: Int) -> String
    {
       // ...
    }
    
  • There is a handy Swift feature called "Tuples". Tuples are groupings of values. We can use them to represent our results from the modulo operation.

    let result = (i % 3, i % 5)
    
  • Now that we are using a Tuple to represent the result, we can easily use a switch to perform the necessary actions.

    switch result
    {
        case (0, 0):
            return "FizzBuzz"
        case (0, _):
            return "Fizz"
        case (_, 0):
            return "Buzz"
        default:
            return "\(i)"
    }
    

    You might be wondering what that underscore is. That _ is used to indicate a discarded value that we don't really care about, the value in the tuple that doesn't really matter to us in the evaluation.

  • Your for loop isn't very "Swift-y". Here's how I would write is so that it calls the function 100 times.

    for number in 1...100
    {
        println(fizzbuzz(number))
    }
    

Answers to your questions

  • The parenthesis in if statements are optional in Swift. Should that be a thing, or should we stick with them?

    This is a very style-oriented question. Style differs from person to person, and is very organic. Since no "style rules" have been put in place, do whatever you are most comfortable with.

  • The curly-braces were optional in Objective-C (and lots of programming languages) but they're not in Swift.

    This was implemented in order to prevent simplified if conditional statements and the class of bugs that associated with them. For example:

    if (someCondition)
        doThisForSomeCondition()
        doThisAsWell()
    if (someOtherCondition)
    // ...
    

    Swift's forced usage of braces eliminated the execution of doThisAsWell() outside of the someCondition conditional statement.

  • Is it okay to let the type be implicitly determined, or should we stick with explicitly declaring the type?

    Whether or not you include the explicit type is a matter of taste. In some contexts it might make your code more readable. However, this is usually not the case; and since the compiler will almost never assign the wrong type to your variable, I usually leave the type to be implicitly determined. Whatever way you decide will not affect speed/efficiency of the code, so that is not a factor.

share|improve this answer
1  
Is there a 'precedence' in the switch operator...? Should the (0,0) case be before the (0,_) and (_,0) cases? –  rolfl Jul 12 at 1:47
    
@rolfl You are correct according to the documentation. Edit has been made. –  syb0rg Jul 12 at 2:17
    
+1 for the for number in 1...100 suggestion. I don't think the tuple and switch implementation is an improvement over the original. Tuples are useful sometimes, but I don't think this is one of those times. I see code duplication; "Fizz" and "Buzz" appear twice now. Also, the tuple solution gets clumsy if you try to extend it to "FizzBuzzPling" as suggested here: codereview.stackexchange.com/questions/56708/… –  GraniteRobert Jul 12 at 2:24
    
@GraniteRobert This is due to the design of the language: "In contrast with switch statements in C and Objective-C, switch statements in Swift do not fall through the bottom of each case and into the next one by default. Instead, the entire switch statement finishes its execution as soon as the first matching switch case is completed, without requiring an explicit break statement." If the design were more C-like, it would be a lot less awkward to extend it. –  syb0rg Jul 12 at 2:36
4  
We're not talking about Ruby--we're talking about Swift. :/ –  nhgrif Jul 12 at 12:06
show 4 more comments

In a for loop in Swift, the parentheses are optional, and from all of Apple's book and sample code, they are usually omitted--you did so yourself on the if statements. Furthermore, Swift has a range operator (two, in fact), so you should use that instead of the manual increments anyway.

for i in 1...100 {
    ...
}

For better or worse, == 0 is the only way to do the comparison in Swift, and many would argue it's easier to read in C/ObjC anyway.

share|improve this answer
add comment

Optional Parentheses

I haven't seen any good reasons to use parentheses where Swift says they are optional. It makes no more sense to use them in an if or for than it does in a simple math expression like foo = (x + y). Or the C incantation that some folks believe has mysterious powers: return (foo);

If you want your Swift to look more like C, go a head an add the optional parentheses. But that seems an unworthy goal to me, and doomed to failure. :-)

Comparison with ==

I have the opposite view; strings are ordinary objects in Swift, the ordinary comparison operators like == seem reasonable to me. Building Objective-C on top of C forced some weird idioms on us. I'm willing to let them go.

share|improve this answer
add comment

A couple of observations around these lines…

if fizzBuzz == "" {
    fizzBuzz += "\(i)"
}
  • This seems like an unjustified use of string interpolation. String(i) would be more direct.
  • Why bother concatenating to an empty string?

With those two changes…

if fizzBuzz == "" {
    fizzBuzz = String(i)
}
share|improve this answer
add comment

I don't know if this is Swift-y or Ug-ly, or maybe both.

I tried to make something similar to OldCurmudgeon's data-centric FizzBuzzPling extension in Java I didn't get far with enum, but found a way to implement it with struct:

struct FizzBuzz
{
    var s: String
    var n: Int

    init(_ s: String, _ n: Int)
    {
        self.s = s
        self.n = n
    }

    func fizz(number: Int) -> String
    {
        return (number % self.n) == 0 ? self.s : ""
    }
}

let fizzBuzzArray = [FizzBuzz("Fizz", 3),
                     FizzBuzz("Buzz", 5),
                     FizzBuzz("Pling", 7)]

func fizzbuzz(number: Int) -> String
{
    var result = ""
    for fb in fizzBuzzArray
    {
        result += fb.fizz(number)
    }
    return (result == "") ? String(number) : result
}

for number in 1...106
{
    println(fizzbuzz(number))
}

The initializer for FizzBuzzArray seems too verbose, but I don't know a way to simplify it.

I borrowed the String(number) suggestion from 200_success.

share|improve this answer
add comment

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.