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.
Performance. Compared to the simplest answer your solution is ~20 times slower. That is huge and completely unacceptable for the task.
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.
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