By Zachary Woods


2019-01-08 15:26:17 8 Comments

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.

7 comments

@Sᴀᴍ Onᴇᴌᴀ 2019-01-08 16:54:29

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...
  • Statements and blocks - If you are going to use block statements then expand them so each statement is on a separate line. Otherwise, just put the single statement without a block. Also use consistent statement termination. The first three conditional blocks don't have a semi-colon after the statement within the block, while the last one (in the else block) does.
  • 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.

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 (var 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) {
    var output = "";
    if (value % 3 === 0) output += "Fizz";
    if (value % 5 === 0) output += "Buzz";
    return output || value;
}
for (var i = 1; i <= 100; i++) {
    console.log(fizzBuzz(i));
}

@Džuris 2019-01-08 22:01:35

There is also the "never use semicolons" school. blog.izs.me/2010/12/… feross.org/never-use-semicolons

@Voile 2019-01-09 09:43:54

@CaseyB I don't think arguing about micro-optimizations like this when OP is a beginner is a good idea at all.

@Michael 2019-01-09 10:34:55

@Voile One million percent. Even if this were a production-ready, enterprise-grade, cloud-based microservice™, readability and maintainability is always the number one concern. The concept of micro-optimizing a JavaScript FizzBuzz is honestly hilarious.

@Mateen Ulhaq 2019-01-09 12:05:58

@Michael FizzBuzz Enterprise Edition begs to differ. :P But FWIW, I believe Casey was merely providing a counterargument for the microoptimization that the answer itself first suggested.

@Juha Untinen 2019-01-09 13:10:25

@MateenUlhaq 260 issues... man, they really need to refactor it, arrange UATs, and use Kanban.

@Tombo 2019-01-09 14:46:36

I think you are just looking for criticisms, and are largely judging style. Because of different styles of different developers, I can find a criticism of any code. If you want common style (which also has its potential downsides) just use a code formatter. Also, making a function "just because" can lead to difficult to maintain code (unnecessarily complex modularization). I typically don't modularize a code block into a function/method until said logic is used by two distinct code blocks, and then the logic needs to be complex enough to warrant it.

@user3067860 2019-01-09 15:17:20

@Tombo If it was my style vs your style, I would agree, but the OP has not used a consistent style (semi-colons sometimes but not always, indentation sometimes but not always). You can get these things with a formatter, sure, so the OP should use one. (Being a beginner, it's reasonable that they don't know this automatically, but it's something to learn.)

@CaseyB 2019-01-09 15:19:43

@Voile Sure! But the "improved" string concatenation version was also more complex. It was more a comment that the added cleverness was misplaced.

@Koray Tugay 2019-01-09 16:04:55

I do not agree with this one: Consider default value.

@Sᴀᴍ Onᴇᴌᴀ 2019-01-11 16:20:35

@KorayTugay okay I have removed that part about the default value. In your opinion, is this an improvement?

@Phrancis 2019-01-11 09:34:28

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));
}

@Hemanshu 2019-01-09 17:07:43

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

@nostalgk 2019-01-09 20:10:28

While I think it's a bit early for a beginning programmer to be learning about things such as memory management (assuming this is one of their FIRST actual programs), I think this is a good answer!

@Sulthan 2019-01-09 23:37:42

This advice screams "premature optimization". Division is not an expensive operation. It's a bit more expensive that addition but it's not really expensive on today's computers. Your mobile phone can do millions of divisions in a fraction of a second. String concatenation is much more expensive, by far. The most important metric in code quality is readability. Also, if you are talking about computational complexity, you should note that your change won't change complexity at all.

@crasic 2019-01-10 03:05:13

The theoretical computational complexity of the underlying division algorithm is practically irrelevant.

@Hemanshu 2019-01-10 13:58:50

@Sulthan The subject computational complexity doesn't take in to account how many operations a machine can perform, it takes into account how many operations/steps are required to solve a problem. While CC of addition i linear, division is quadratic. Dividing only by 3 and 5 would change the complexity and results of those can be saved in a bool variable which can be evaluated in constant time complexity.

@Blindman67 2019-01-10 14:08:53

On a modern CPU integer division takes 1 CPU cycle, the JS version of % a little longer. JS will use 32bit signed ints when it can so the operation is insignificant in comparison to just calling a function. BTW the linked computational complexity page has nothing to do with how CPUs ALU and FLU process the basic math operations.

@Sulthan 2019-01-10 14:12:58

@Hemanshu I think you misunderstand what time complexity is. In time complexity the important metric is the amount of data, in this case the length of the for cycle. Everything inside is a constant, either with 2 or 3 divisions. Still a constant. The resulting time complexity is still O(n).

@Hemanshu 2019-01-10 15:02:39

@Sulthan, i don't want to get into an argument here, you are completely ignoring the division fact.

@Hemanshu 2019-01-10 15:03:49

@Blindman67 could you point me to the direction of the algorithm used for divisions

@Blindman67 2019-01-10 15:49:51

CPUs do these OPs using a combination of hardware and internal instruction sets (or pure hardware in simpler CPUs). There are many tricks that can be used so the logic steps to perform an OP will depend on the CPU. The operations are separated from the CPU and performed on dedicated hardware, called the, Arithmetic Logic Unit (ALU) , and Floating point Logic Unit (FLU or FPLU). Because these units CPU, ALU, FLU are independent they can often perform operations in parallel. Making it very hard to know what is most performant in high level languages such as Javascript. Google CPU ALU for more.

@crasic 2019-01-11 20:48:47

@Hemanshu the underlying hardware algorithm does scale based on your estimate of complexity. However, Hardware dividers have fixed input lengths and guaranteed times rounded to the next clock cycle. As such the amount of time to divide 4/2 and 400000/200000 is the same because both are fed in as hardware words. Effectively for all word sized inputs the time is constant for all ALU operations. If you are implementing arbitrary length division algorithm then this you must apply the theoretical computational complexity. For hardware division it is fixed time for all operations.

@crasic 2019-01-11 20:50:44

Put another way if you are designing the CPU a 64 bit ALU divider will generally take 4 times longer than a 32 bit ALU divider and this impacts your timing and signal propogation (pipelining), but to the programmer this is irrelevant because the operation is constant time and constant input. 1 tick vs 2 for addition/division is noting compared 100 for cache miss due to memory access (even though this is "O(1)"). Complexity is only useful to compare the relative performance of different algorithms vs input size, once everything has been fixed in place only absolute time matters

@Michael 2019-01-09 10:22:58

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.

@Michael 2019-01-10 16:15:15

@PatrickRoberts True. I wanted to focus more on the approach to solving the problem than those things. Buuut I did correct everything else (I couldn't help myself) so yeah I've made that change as well. Thanks

@Patrick Roberts 2019-01-10 16:23:09

You forgot the equality in the ternary expression. I tried to edit myself but I can only make edits of at least 6 characters :|

@Steve 2019-01-09 16:22:49

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.

@GrumpyCrouton 2019-01-09 21:00:55

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);
}

@Paul 2019-01-08 15:51:58

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));
}

@Moby Disk 2019-01-09 18:59:14

"Returning early" is often called "multiple exit points" and is sometimes considered poor practice.

@nurettin 2019-01-10 11:22:59

"Returning early" is often called "validation" and is often considered good practice.

@Tim B 2019-01-10 17:25:45

Returning early has many names and people get very worked up about whether their way is best.

@wizzwizz4 2019-01-12 12:44:47

@MobyDisk That advice is originally from Assembly, and referred to "places to exit to", not "places to exit from". That removes the "it's always been done this way, so there must've been a reason" argument. Before applying your revised version of this advice to a particular scenario, check whether it actually makes sense. In this case, it doesn't.

Related Questions

Sponsored Content

0 Answered Questions

A Structured FizzBuzz

1 Answered Questions

[SOLVED] FizzBuzz in Common Lisp

3 Answered Questions

[SOLVED] FizzBuzz in Brainfuck

  • 2014-07-18 11:22:57
  • Simon Forsberg
  • 9123 View
  • 83 Score
  • 3 Answer
  • Tags:   fizzbuzz brainfuck

2 Answered Questions

[SOLVED] Fizz, Buzz, or FizzBuzz?

10 Answered Questions

[SOLVED] Yet another boring FizzBuzz

1 Answered Questions

[SOLVED] Ultimate FizzBuzz

4 Answered Questions

[SOLVED] Extensible and testable FizzBuzz

2 Answered Questions

1 Answered Questions

[SOLVED] PseudoBrain FizzBuzz Thoughts

  • 2013-12-06 03:34:20
  • Mathieu Guindon
  • 188 View
  • 5 Score
  • 1 Answer
  • Tags:   c# fizzbuzz ai

2 Answered Questions

[SOLVED] Fizz Buzz interview question code

Sponsored Content