Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

Is this a bad solution to the FizzBuzz thing?

I decided to make it print "Fizz Buzz" for the numbers divisible by 3 and 5 just for practice/understanding.

for (var n = 1; n <= 100; n++) {
var output = "";
if (n % 3 == 0)
    output += "fizz";

if (n % 5 == 0)
    output += "buzz";

if (n % 5 == 0 && n % 3 == 0)
    output = "fizz buzz";

console.log(output || n);
};
share|improve this question
up vote 0 down vote accepted

Indentation:

You're missing indentation in your code, you should space out each level of indentation.

For example:

for (var n = 1; n <= 100; n++) {
var output = "";
if (n % 3 == 0)
    output += "fizz";

would become:

for (var n = 1; n <= 100; n++) {
    var output = "";
    if (n % 3 == 0)
        output += "fizz";

Use braces

You should wrap your code in braces, so you don't cause any bugs unknowingly:

For example, you (or someone in future that maintains your code) might think this wouldn't log anything, however, "omg" is logged.

if (5 != 5)
    console.log("wow");
    console.log("omg")

Even Apple got caught out by one of these bugs.


Unnecessary logic:

In your block, you check whether the number is divisible by 3, then by 5, then by 3 and 5. You don't need to perform the last check of 3 and 5, the first two make up that one.


Leaving you with:

Implementing all those changes, you would end up with:

for (var n = 1; n <= 100; n++) {
    var output = "";
    if (n % 3 == 0){
        output += "fizz";
    }
    if (n % 5 == 0){
        output += "buzz";
    }
    console.log(output || n);
};
share|improve this answer
    
Thanks for your detailed answer. The only reason I added that last bit was to see if the code would still function. It also outputs "fizz buzz" instead of "fizzbuzz" for numbers divisible by both. Unnecessary, just there for my personal learning. – Alex Gray Mar 31 at 4:32

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.