2
\$\begingroup\$

I've been learning JavaScript for a few weeks and thought to give the ol' fizzbuzz another try. My goal was for expandability. In other words, I wanted to be able to add more keywords without having to rewrite parts of the code.

I tried to add comments to clarify some of the more unorthodox parts of my code. However, I still feel my code is somewhat terse... Is it better to just write the plain old for-loops for readability and support of IE?

I also opted to check if the string existed instead of checking if the length was greater than zero. Are there any pitfalls I might fall into just checking for existence? I mean if my key were false it would fail, however that would never happen as all the key values are strings anyways.

function fizzBuzz(divisors = {
        fizz: 3,
        buzz: 5
    }, maxNumber = 100) {
        /* Takes in a object of divisors of the form
           divisors = {
               name: value,
           };
           and an integer. Loops through the list if number below or equal
           maxNumber. If a number is divisible by one of the values, print the name.
          */
        for (var num = 1; num <= maxNumber; num++) {
            var str = "";
            // Loops through the keys of the divisors
            Object.keys(divisors).forEach(function(key)  {
                // Ternary expression, if divisible append key else do nothing
                str += num % divisors[key] == 0 ? key : '';
            });
            // If anything got appended print string else num
            console.log(str ? str : num)
        }
    }
    
    fizzBuzz()

\$\endgroup\$
1
\$\begingroup\$

What if I wanted every 7 number to also add fizz, so 105 would be fizzbuzzfizz? You can't do that in your code, since you are using the string as the (unique) key and the number as the value. Instead, use the number as the key and the string as the value.

While it will work in most cases, the order of properties in an object is not guaranteed. I would change this to either use an array of object or a map.

What if you wanted just the result form a single number? Or make a function that found the first number that gives fizzbuzz? Right now you are printing the result, so you can't do anything else with it. Make a separate function to get the result, which you can also use for other things.

function fizzBuzz(divisors = new Map([[3,'fizz'],[5,'buzz']]), maxNumber = 100) {
  for (var num = 1; num <= maxNumber; num++) {
    console.log(fizzValue(divisors, num));
  }
}

function fizzValue(divisors, num) {
  var str = "";
  divisors.forEach((value, key) => {
  	str += num % key == 0 ? value : '';
  });
  return (str ? str : num);
}

fizzBuzz();

\$\endgroup\$
  • \$\begingroup\$ While the other answers were able to point out my many mistakes, this answer really showed me a great way of solving the problem, thanks! \$\endgroup\$ – N3buchadnezzar Dec 3 '17 at 7:52
2
\$\begingroup\$

Review answer to FizzBuzz

If I gave this test and I received the answer given I would not give it a full pass. It does work which is a pass, but compared to an expected answer it performs poorly.

Granted you are stretching your programing legs which is great and the answer is good (not great) and I will say well done. Don't take the review as a negative on your coding skills, many people can't manage a for loop at the end of 1st year CS study. For a few weeks you are well ahead of the curve.

Comments are dangerous.

First a side note. We are all coders and all read code with ease and don't need comments to understand code.

For example

// Loops through the keys of the divisors
Object.keys(divisors).forEach(function(key)  {

Someone that needs the comment to understand the next line would never be reading the code in the first place.

Comment only abstract concepts that are not self evident in the immediate context, or comment for machine readers like linters, documenters, builders... etc.

Comments are dangerous, as they are never vetted by compilers/interpreters and the debugging/testing cycle, they remain forever in the code. You may change some of the code and forget to change the comment. That comment remains in the code and one day when someone new comes to the code they will be faced with contradicting information.

What is wrong with the answer?

There is a trap in programming that is easy to fall into and that is over complicating the solution. And you have given a classic example of over engineering a simple solution.

Complexity has many downsides.

  1. Performance. Compared to the simplest answer your solution is ~20 times slower. That is huge and completely unacceptable for the task.

  2. New and unspecified behaviour. When engineers design programs they are very careful to describe the behaviour. That behaviour is part of a much larger scope. You have added arguments where none were specified, Your code runs many times slower than would be expected. Both these things can case problems.

  3. Complexity is costly, time to write, time to test, time to debug, and time to understand. In a competitive market time is the bottom line.

Assuming you wrote to spec.

I said above that your code was good not great. Lets assume you wrote the function to a specification that required the arguments and flexibility. What could be done to improve the function.

Minor issues. The vars num and str should have been created outside the loop at the top of the code, or where they are as block scoped variables using let.

Performance is king

In my book (and not everyones) if all things are equal then performance is the indicator of the best solution.

The major problem in the code are the lines

Object.keys(divisors).forEach(function(key)  {
    str += num % divisors[key] == 0 ? key : '';
});

inside a for loop which has a major impact on performance. Golden JS rule "Never define a function inside a loop."

The function Object.keys creates a new array each time it is called. That array needs memory and CPU time to fill. You create 100 identical arrays, when you only need one.

The function inside the forEach like the array is a unique instance. Each time it is created inside the loop it creates a new context and closure which requires memory and CPU time. Like the keys array you only need one instance of the function with one context and one closure.

Create the array and function outside the loop to reduce overhead.

Above I mentioned the performance was ~20 times slower. My benchmarker gave your code a score of 19.5 which is a value proportional to execution time averaged out over many calls. This value only makes sense in comparison to other functions. (19.5 compared to basic version 1.4 on firefox and 13.4 to 0.8 on Chrome. (1))

Keeping to your design and with the following modifications in steps moving first the Object.keys functions out of the loop (fizzBuzzA) and then creating the function outside the loop (fizzBuzzB) and finally replaced the array iterator with a for loop (fizzBuzzC) and benchmarked the results. See bottom of answer for code.

The results on firefox (win10)

=======================================
Performance test. : "Fizz Buzz"
Use strict....... : true
Data view........ : false
Duplicates....... : 4
Cycles........... : 63
Samples per cycle : 100
Tests per Sample. : 100
---------------------------------------------
Test : 'fizzBuzz'
Mean : 18670µs ±568µs (*) 1593 samples
---------------------------------------------
Test : 'fizzBuzzA'
Mean : 11595µs ±684µs (*) 1597 samples
---------------------------------------------
Test : 'fizzBuzzB'
Mean : 6610µs ±481µs (*) 1598 samples
---------------------------------------------
Test : 'fizzBuzzC'
Mean : 4243µs ±595µs (*) 1512 samples
-All ----------------------------------------
Mean : 10.355ms 
Totals time : 65234.870ms 6300 samples
(*) Error rate approximation does not represent the true variance.

Normalised the results are 18.7, 11.6, 6.6, and 4.2. You can see how the efficiency increases as you change the code. And on Chrome 11.7, 6.5, 5.0,and 3.0. I am surprised this ran faster on Chrome as it has been a poor performer compared to firefox also the variance (error bars) on Chrome are only 1/3rd of FF.

Thus functionally identical with a few minor modifications in layout you can get major performance benefits.

The 4 functions as tested.

Not a runnable snippet

/* Modified to return the string rather than output to console */
// OP's original with modified output
function fizzBuzz(divisors = {
  fizz: 3,
  buzz: 5
}, maxNumber = 100) {
  var result = "";

  for (var num = 1; num <= maxNumber; num++) {
    var str = "";
    Object.keys(divisors).forEach(function(key) {
      str += num % divisors[key] == 0 ? key : '';
    });
    result += str ? str : num;
  }
  return result;
}
//======================================================
function fizzBuzzA(divisors = {
  fizz: 3,
  buzz: 5
}, maxNumber = 100) {
  var result = "";
  const keys = Object.keys(divisors);

  for (let num = 1; num <= maxNumber; num++) {
    let str = "";
    keys.forEach(function(key) {
      str += num % divisors[key] == 0 ? key : '';
    });
    result += str ? str : num;
  }
  return result;
}
//======================================================
function fizzBuzzB(divisors = {
  fizz: 3,
  buzz: 5
}, maxNumber = 100) {
  var result = "";
  var str, num; // needed outside loop for function to close over.
  const keys = Object.keys(divisors);
  const divisorTest = (key) => {
    str += num % divisors[key] == 0 ? key : ''
  };

  for (num = 1; num <= maxNumber; num++) {
    str = "";
    keys.forEach(divisorTest);
    result += str ? str : num;
  }
  return result;
}
//======================================================
function fizzBuzzC(divisors = {
  fizz: 3,
  buzz: 5
}, maxNumber = 100) {
  var result = "";
  const keys = Object.keys(divisors);
  for (let num = 1; num <= maxNumber; num++) {
    let str = "";
    for (const key of keys) {
      str += num % divisors[key] == 0 ? key : '';
    }
    result += str ? str : num;
  }
  return result;
}



//======================================================
/* Basic version used in first comparison, as i write it (could be better?) */
function fizzBuzz1() {
  const f = "fizz";
  const b = "buzz";
  var i, out;
  var result = "";
  for (i = 1; i <= 100; i++) {
    out = i % 3 === 0 ? f : "";
    result += out + (i % 5 === 0 ? b : i);
  }
  return result;
}

(1) Note the code was changed to return results as a string, and not output to the console as console output messes up benchmarking results

\$\endgroup\$
  • \$\begingroup\$ Fantastic answer! I really did not realise I kept remaking the same object over and over again. You mentioned over-engineering. Do you think my solution is over-engineered? If you were to create a function to do fizzubuzz with arbitary number of inputs, what would be the not-over-engineered approach? \$\endgroup\$ – N3buchadnezzar Dec 3 '17 at 7:52
  • \$\begingroup\$ @N3buchadnezzar If fizzbuzz required arbitrary inputs then you did not over engineer the solution. \$\endgroup\$ – Blindman67 Dec 3 '17 at 10:41
1
\$\begingroup\$

My feelings on your code:

  1. Wow, that's cool way to expand fizzbuzz!
  2. Using forEach is awesome. I would way rather read that than a for loop.
  3. Using a ternary in a situation where you might do nothing seems kind of funny to me. I would prefer

    if (num % divisors[key] === 0) { str += key; }

    or even if (num % divisors[key] === 0) str += key; if you like it on one line, because it makes it more obvious to the reader that you are doing nothing in the false case. IMO its better to have more code that needs no comment than have less code with the necessity of a comment to explain it.

  4. From a functionality standpoint, I think console.log(str ? str : num) is totally ok, but from a readability standpoint some people might prefer checking the length.

\$\endgroup\$
1
\$\begingroup\$

Make sure that your code always generates the keywords in the correct order. The task is called "fizzbuzz" and therefore should never output "buzzfizz". I know that in PHP the order in an array is guaranteed, but in JavaScript I don't know of any such guarantee. Just try with 100 keywords to be sure.

These keywords can even be assigned the same number. So {f:3, izz:3, b:5, uzz:5} would still have to work. That example has a 23/24 chance of producing unwanted output, compared to your current code, which has a 1/2 chance.

\$\endgroup\$
  • \$\begingroup\$ The order is not guaranteed across browsers, Object.keys uses the same algorithm as for...in mdn \$\endgroup\$ – Gerrit0 Nov 30 '17 at 3:33
0
\$\begingroup\$

The formatting/indentation is a bit off. It maybe partially due to pasting it here.


I'm not that enthusiastic about using forEach here as @setholopolus is. It's a case where a simple for ... in loop would work fine too, and which would be shorter and better readable:

for (key in divisors) {
  str += num % divisors[key] == 0 ? key : '';
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.