FizzBuzz in Javascript

Question

I'm sure everyone here knows what FizzBuzz is. I would like constructive criticism for my solution.

I'm a beginner to programming as a whole and this isn't my first solution, but it's what I think is my best solution. I'd like to see if this is a reasonable solution, or is just plain terrible.

for(var i=1;i<=100;i++) {
var output = "";

    if(i % 15 == 0) {output = "FizzBuzz"}
    else if(i % 3 == 0) {output = "Fizz"}
    else if(i % 5 == 0) {output = "Buzz"}
    else {output = i;}

console.log(output);
}

The output is correct. It's your standard FizzBuzz output.

Answer 1

I'd like to see if this is a reasonable solution, or is just plain terrible.

I wouldn't say it is "terrible" - mostly because it works and doesn't appear to be very inefficient. However, there are some improvements that can be made.

• use strict equality comparison - i.e. === when comparing values. That way it won't need to convert the types.

• Style Guide Consider following a style guide. Many common style guides advise separating keywords with a space - e.g. if ( instead of if(.

• Use consistent indentation The first and last lines within the for loop are not indented, while the other lines between them are, though maybe it was the case that your code was indented properly but when you pasted here it was thrown off because of the markdown formatting...

• Abstract logic into a function As Paul's answer suggests: you can put the core logic into a function that returns the output but doesn't handle outputting the value (e.g. to the console). This allows such code to be atomic and testable - congruent with the Single Responsibility Principle. Also, the return statement can eliminate the need for else keywords within a function. One drawback is that calling a function on each iteration may slow down operations but should be negligible for a set of numbers 1 through 100.

Updated Code

Consider the modified code below, utilizing the feedback above. I also used this code in the code contained in my recent question: Fizzbuzz with dynamic height container

Note: the inline console in the snippets is truncated to ~50 lines, but the complete console log should be visible in your browser console.

function fizzBuzz(value) {
    if (value % 15 === 0) { return "FizzBuzz"; }
    if (value % 3 === 0) { return "Fizz"; }
    if (value % 5 === 0) { return "Buzz"; }
    return value;
}
for (let i = 1; i <= 100; i++) {
    console.log(fizzBuzz(i));
}

One option I did consider is to minimize the number of modulo operations, append to the output string instead of outputting it. If you are trying to optimize for speed, this might not be the approach to take because appending to the string may be much slower than doing an extra couple modulo operations. Try a comparison in this jsPerf test.

function fizzBuzz(value) {
    let output = "";
    if (value % 3 === 0) { output += "Fizz"; }
    if (value % 5 === 0) { output += "Buzz"; }
    return output || value;
}
for (let i = 1; i <= 100; i++) {
    console.log(fizzBuzz(i));
}

Answer 2

One of the problems is that the case where you check i % 15 (i.e. i is a multiple of 3 and 5) is unnecessary. You have the concept of 3 and 5 repeated, and the concept of Fizz and Buzz repeated.

This is not currently much of a problem but suppose someone asks you to extend your program to print "Jazz" when i is a multiple of 7. Using your current strategy we now need a case where i is a multiple of:

1. 3
2. 5
3. 7
4. 3 and 5
5. 3 and 7
6. 5 and 7
7. 3 and 5 and 7

It would look something like this:

for(var i = 1;i <= 100; i++) {
    var output = "";

    if(i % 105 == 0) {output = "FizzBuzzJazz"}
    else if(i % 15 == 0) {output = "FizzBuzz"}
    else if(i % 35 == 0) {output = "BuzzJazz"}
    else if(i % 21 == 0) {output = "FizzJazz"}
    else if(i % 3 == 0) {output = "Fizz"}
    else if(i % 5 == 0) {output = "Buzz"}
    else if(i % 7 == 0) {output = "Jazz"}
    else { output = i; }

    console.log(output);
}

See how quickly that got out of hand? If we add a fourth word it becomes even worse.

If we use a different strategy by appending text to the output variable, we can get away with having as few conditions as we have words.

for(var i = 1; i <= 100; i++) {
    var output = "";
    if (i % 3 === 0) {output += "Fizz";}
    if (i % 5 === 0) {output += "Buzz";}
    if (i % 7 === 0) {output += "Jazz";}

    console.log(output === "" ? i : output);
}

(I've fixed a few other things as suggested in Sam's answer)

One thing that might be new to you here, if you're a beginner, is that the expression used as the argument for console.log or called the conditional or ternary operator. Ours says that if the output is blank (i.e. not a multiple of 3, 5 or 7) then print i, else print the string that we've compounded.

The ternary operator can always be replaced by an if-statement if you're not yet comfortable with it.

Answer 3

I think it's fine as is, though some folks like to return early instead of using a series of if..else. For example:

function calc(i) {
  if(i % 15 == 0) return "FizzBuzz";
  if(i % 3 == 0) return "Fizz";
  if(i % 5 == 0) return "Buzz";
  return i;
}

for(var i=1;i<=100;i++) {
  console.log(calc(i));
}

Answer 4

Expanding on Michael's answer, you could also create an object with all the words you want, with the key being the number your input must be divisible by, to make this more dynamic and a bit more future proof (A lot easier to add a new entry to the object versus adding more lines of code), and the object itself can also be dynamically generated.

divisions = {3: "Fizz", 5: "Buzz", 7: "Jazz"};

for(var i = 1; i <= 100; i++) {
    var output = "";

    for (var x in divisions) {
        if(i % x == 0) output += divisions[x]; 
    }

    console.log(output == "" ? i : output);
}

Answer 5

I tend to be somewhat "literalist", at least at first, when implementing algorithms. So I would start with this, and then optimize from there. I like that it keeps things very clear.

const calcFizzBuzz = function(num) {
  let result = "";
  if (num % 3 === 0) {
    result += "Fizz";
    if (num % 5 === 0) {
      result += "Buzz";
    }
  } 
  else if (num % 5 === 0) {
    result += "Buzz";
  } 
  else {
    result = num;
  }
  return result;
}

for (let i = 0; i < 100; i++) {
  console.log(calcFizzBuzz(i));
}

Answer 6

From a maintenance stand point, I think it's better that you check for 3 and 5 instead of checking for 15. The issue with checking for 15 is that when you come back to your code 6 months from now, you won't realize that you were just printing fizzbuzz when something was divisible by both 3 and 5. That might matter in the future, but I think that's something worth talking about in an interview.

Answer 7

Though your code is fine, another thing to look is amount of division happening the code, Division is computationally expensive operation and as @Michael has suggested that it is redundant. So always try to minimise the multiplication and division operation in your code.

You have mentioned that you are a beginner programmer. I believe it is a good practice to start familiarising your self with topics like Computational Complexity.

Look here for computational complexity of mathematical functions