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.

In my quest for learning Objective-C, I have created the common FizzBuzz code in it. This is due to a suggestion/challenge from @nhgrif to re-implement anything he does in Swift in Objective-C.

#import <Foundation/Foundation.h>

int main(int argc, const char* argv[])
{
    @autoreleasepool
    {
        for (int i = 1; i <= 100; ++i)
        {
            if (i % 15 == 0) NSLog(@"FizzBuzz");
            else if (i % 3 == 0) NSLog(@"Fizz");
            else if (i % 5 == 0) NSLog(@"Buzz");
            else NSLog(@"%d", i);
        }
    }
}
share|improve this question

5 Answers 5

up vote 5 down vote accepted

I'd probably code the logic inside the loop this way:

if (i % fizz_divisor == 0) {        
    if (i % buzz_divisor == 0) {
        NSLog(@"FizzBuzz");
    } else {
        NSLog(@"Fizz");
    }
} else if (i % buzz_divisor == 0) { 
    NSLog(@"Buzz");
} else {
    NSLog(@"%d", i);
}

This invokes the % operator at most two times, rather than possibly three times. Of course now you have to define the constants fizz_divisor and buzz_divisor, but it is easier to make the program continue to work correctly if you later decide that the divisors should be something other than 3 and 5. It's also a pattern that's relatively easily extended if you want to implement a third divisor (e.g. to play "Fizz Buzz Ding").

share|improve this answer
1  
I like this. It does sacrifice some readability for performance, but I really like that you only have to call the mod operator 3 times instead of the four times in my answer. –  ckuhn203 Jul 12 at 15:32
2  
I only had to write it three times. It gets called twice. We could modify this code a little to write the mod operator only two times, but that might not be good for readability and it would do nothing for performance. –  David K Jul 12 at 15:49
    
Yup. You're right @DavidK. I misspoke. –  ckuhn203 Jul 12 at 15:50
    
It was a good point, though. The number of expressions we have to write can matter too. –  David K Jul 12 at 16:00
    
@nhgrif: Yes, that's how to write just two % operations. It requires you to make a mental connection between the logical variables used inside the if and the code where those variables are defined, but if the if conditions were much more complicated I would do that. –  David K Jul 12 at 16:03

You have no curly braces!

You should be sure to include curly braces. For one, it prevents mistakes when you want to go back and add some logic into one of these if statements in the future, and for two, since we're talking about Objective-C and you're almost certainly using Xcode, it makes the code collapsable.


I know, for some reason, that main drops its opening curly brace to the next line. I don't know why, and usually, I could care less because honestly, we spend almost no time in main when developing iOS/OSX projects, however... by default the @autoreleasepool leaves its opening brace on the same line, and the autocomplete on for statements do this as well.

It's a style thing, yes, but in Objective-C, we prefer the opening brace on the same line. And there's actually a good reason for this... If you notice in the left-hand margin of your code, just to the right of the line numbers, there are differing shades of gray. Clicking these shades of gray collapses different sections of your code.

Compare the difference of collapsing a for loop with the brace on the same line versus the brace on it's own line. To me, the latter looks silly and it takes up an extra line!

So all in all this looks a bit better:

int main(int argc, const char* argv[]) {
    @autoreleasepool {
        for (int i = 1; i <= 100; ++i) {
            if (i % 15 == 0) {
                NSLog(@"FizzBuzz");
            } else if (i % 3 == 0) {
                NSLog(@"Fizz");
            } else if (i % 5 == 0) { 
                NSLog(@"Buzz");
            } else {
                NSLog(@"%d", i);
            }
        }
    }
}
share|improve this answer
    
Good points about the collapsible code using braces, that's something I didn't think of when eliminating them. –  syb0rg Jul 12 at 15:01

I don't like that you've implemented logic in Main. Move the logic into a FizzBuzz method and call that from Main. (Even though it seems really silly for such a simple program, it's not best practice to have it there.)

I'm also not a fan of hard coding 15 as the "FizzBuzz" case. What if we decide that Fizz should be 2 and Buzz should be 5? Now 15 as your first case makes no sense and Mr. Maintainer has to figure out why 15 is a case at all. It's easier to understand and maintain if (i % 3 == 0 && i % 5 == 0) {.

This might be clearer if you consider 15,3, & 5 for what they really are. They're magic numbers and as such should be stored as constants. I don't know Obj-C, so I'll provide a pseudo code example of how I think it should be done.

const int fizz = 3
const int buzz = 5

if (value % fizz == 0 && value % buzz == 0) {
    Print("FizzBuzz")
} else if (value % fizz == 0) {
    Print("Fizz")
} else if (value % buzz == 0) {
    Print("Buzz")
} else {
    Print(value)
}

This allows us to easily update the conditions.

Or alternatively (thanks to Nhgrif and David K)

int fizzbuzz = lcm(fizz,buzz);
for (value = 1; value <= 100; value++) { 
    if (value % fizzbuzz == 0) {
        Print("FizzBuzz")
    } 
    //etc.
}

Where lcm() is some function that returns the lowest common multiple. At that point we may be overthinking the problem, but it does result in a robust solution.

share|improve this answer
1  
Either one could work, but I guess that really also depends on the language. I never liked having to repeat the variable anyway, but that's just me. –  Jamal Jul 12 at 15:20
4  
value % (fizz * buzz) == 0? –  nhgrif Jul 12 at 15:51
3  
Technically, if you want to be sure it works for different values of fizz and buzz, better write if (value % lcm(fizz, buzz) == 0) where lcm is a function that returns the least common multiple of its arguments. –  David K Jul 12 at 16:07
1  
Okay, thanks. That's a good point. And in which case, we'd want to calculate and set the lcm before the loop rather than calculate this unchanging value on each iteration of the loop. –  nhgrif Jul 12 at 16:11
3  
And at this point, we're all overthinking FizzBuzz and have failed the interview question. =;)- –  ckuhn203 Jul 12 at 16:12

I'm no Objective-C expert, but it seems to me that since you are using neither argc nor argv that you could use int main(void).

Also, you could alternatively store the results of (i % 3 == 0) and (i % 5 == 0) and use those within the if statement.

It also seems that a return 0 might be needed at the end of main().

share|improve this answer
1  
In comment to your last point, I assumed that Objective-C inherited and abided by the C specification, specifically in C99 & C11 §5.1.2.2(3): "...reaching the } that terminates the main() function returns a value of 0." –  syb0rg Jul 12 at 14:59
    
@syb0rg: You might well be right. Lacking a standard specification for Objective-C is a well-known problem. –  Edward Jul 12 at 15:07
2  
If we're going to go about removing bits of main that were auto-generated, we could remove the @autoreleasepool too, since there's no objects in the code. –  nhgrif Jul 12 at 15:56

If we want to sacrifice readability in favor of performance, we might like something like this:

const int kFizz = 3;
const int kBuzz = 5;

for (int i = 1; i <= 100; ++i) {
    NSLog(@"%@",
        i % kFizz ? (i % kBuzz ? @"FizzBuzz" : @"Fizz") :
                    (i % kBuzz ? @"Buzz"     : @(i))
    );
}

The key to understanding this solution is that the %@ placeholder calls the description method on whatever argument it is sent. With NSString, the description is exactly what you'd expect. The tricky part here is @(i). The @(i) is modern Objective-C syntax which instantiates an NSNumber object, and calling the description method on an NSNumber object is almost equivalent to using the %i string format placeholder for an integer.

share|improve this answer
    
I usually align the two branches of a ?: operator to start in the same column when they occur on separate lines. That might improve readability here. –  David K Jul 12 at 16:55
    
@DavidK Can you edit to show me how you mean? I typically don't nest ternary operators, and was trying to indent etc. to make it most readable but couldn't figure it out. –  nhgrif Jul 12 at 17:29
    
Done. I also added a line of text because SO didn't believe that my other edits added up to 6 characters, and it wouldn't let me submit them without adding something else. –  David K Jul 12 at 17:46
    
This is Code Review not Code Golf. –  ckuhn203 Jul 12 at 18:18
2  
I was under the understanding that efficiency is a perfectly reviewable topic. This is the most efficient method. It doesn't sacrifice readability for the sake of fewer characters--only for better performance. –  nhgrif Jul 12 at 18:23

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.