Badger's FizzBuzz

Question

I love the badger's song (YouTube), so I wrote a FizzBuzz about it.

The thing is, I've been writing Javascript for years but I'm still not sure about the best practices. So FizzBuzz seemed a simple trial for me to write some code and make it reviewed!

function FizzBuzz(limit){
    var _limit = limit;

    var fizzBuzzContent = 
    [
        {"modulo":3, "word":"Badger"},
        {"modulo":5, "word":"Mushroom"},
        {"modulo":15, "word":"Ooohhh it's a snaaaakkkeeee"}
    ];

    this.calculate = function(){
        for(var counter = 1; counter <= _limit; counter++) {
            var line = counter.toString();

            fizzBuzzContent.forEach(function(e){

                if(counter % e["modulo"] === 0){
                    line = e["word"];
                }
            });

            console.log(line);
        }
    }
}

It would be called this way:

var f = new FizzBuzz(100);
f.calculate();

Is there something in this JavaScript object that's wrong?

Answer 1

A few of things (apologies for repeating points from other answers; I wrote this earlier but didn't have a chance to finish it):

• Didn't know you could "love" that song. But you can certainly be brainwashed by it. See a doctor.

• Perhaps BadgerMushroom would be a more appropriate name?

• I don't think this needs to be a constructor. It could just be a plain function, which would be simpler.

• Constructor or not, you don't need to copy the limit argument to _limit. It's makes no difference (the argument is effectively already a local variable, and numbers are passed by value, not by reference, so they don't need cloning or stuff like that to avoid side-effects).

• calculate is not the greatest name. Yeah, it does do some calculation, but its main output is log messages. So perhaps print or similar would be a better fit.

• You don't need to quote keys in an object unless they contain spaces or other non-valid literals. Just writing { modulo: 5, word: "..." } works fine. (You can quote them, of course, which is why JSON, where quotes are required, is valid JavaScript.)

  And since those keys work as regular property names, you can just write e.modulo and e.word - no need for brackets.

  Though for something so self-contained as this, I'd be fine with just nested array instead. True, you have to write [0] and [1] instead of .modulo and .word, but it's not as repetitive to define the lookup.

• Instead of forEach, I'd stick to a simple old for loop - that way, you can break out of it when you get a match.

• No need for toString on a number. It'll print the same.

So with that, something like this would do the trick too:

function badgerMushroom(limit){
    var line,
        words = [
            [15, "Ooohhh it's a snaaaakkkeeee"],
            [5, "Mushroom"],
            [3, "Badger"]
        ];

    for(var n = 1 ; n <= limit ; n++) {
        line = null;
        for(var i = 0, l = words.length ; i < l ; i++) {
            if(n % words[i][0] === 0) {
                line = words[i][1];
                break;
            }
        }
        console.log(line || n);
    }
}

Answer 2

1. I don't see the point of introducing a private variable _limit when its value is never written to. Why not just use limit from the arguments?
2. Using variable names such as e are meaningless
3. fizzBuzzContent can be modified to use direct array indices, rather than store an array of objects:

var fizzBuzzContent = [];
fizzBuzzContent[3] = "Badger";
fizzBuzzContent[5] = "Mushroom";
fizzBuzzContent[15] = "Oooh";

Personally, I think this looks more elegant, but your mileage may vary.

4. Using .toString() on counter isn't necessary, unless you don't want blue numbers in the developer console
5. Spacing around brackets makes code look cleaner and can aid readability
6. Preincrementing is proven to be slightly faster than postincrementing, but with only 100 iterations, it's meaningless. Still may be worth knowing!

With these suggestions, and without changing any logic, this is FizzBuzz:

function FizzBuzz(limit) {
    var fizzBuzzContent = [];
    fizzBuzzContent[3] = "Badger";
    fizzBuzzContent[5] = "Mushroom";
    fizzBuzzContent[15] = "Ooohhh it's a snaaaakkkeeee";

    this.calculate = function() {
        for (var counter = 1; counter <= limit; ++counter) {
            var line = counter;
            fizzBuzzContent.forEach(function(word, index) {
                if (counter % index === 0) {
                    line = word;
                }
            });
            console.log(line);
        }
    }
}

console.clear();
var f = new FizzBuzz(100);
f.calculate();

Answer 3

Storing data inside an object, inside an array is extraneous, when you're using key values for everything.

Try this kind of data structure instead:

var fizzBuzzContent = {
    "3": "Badger",
    "5": "Mushroom",
    "15": "Ooohhh it's a snaaaakkkeeee"
}

And instead of implicitly storing this layout inside the function, let's make it dynamic and add the 'options' as a parameter, defaulting to the original FizzBuzz if you don't provide your own options. Additionally defaulting to the original 30 limit from the question.

options = options || {
    3: "Fizz",
    5: "Buzz",
    15: "FizzBuzz"
};
limit = limit || 30;

---

this.calculate: It's not really an accurate name, there's not a 'calculation' done, you just start the FizzBuzz process.

counter: Really just i or index works fine.

---

forEach: Some people aren't particularly a fan of forEach and if your build's target audience can't support ES5, then you either need to use a polyfill or something else.

Personally, I'm an advocate of forEach.

As it's a prototype of Array, you can't directly use objects.

But you just suggested an object...

Object has a handy function on it that gets all the keys in the object as an array. By traversing this, you can use the first parameter only in forEach:

Object.keys(options).forEach(function(key){
    ...
}

---

line = counter.toString(): This is JavaScript, making a variable stay in the same type is not a feature of the language, you needn't worry if line becomes an integer, it won't cause issues.

---

Instead of assigning line you could just use a ternary statement return/console.log instead:

console.log(
    index % key === 0
    ? options[key]
    : index
);

---

Putting that together:

function FizzBuzz(limit, options){
    limit = limit || 30;
    options = options || {
        3: "Fizz",
        5: "Buzz",
        15: "FizzBuzz"
    };
    this.startLoop = function(){
        for(var index = 1; index <= limit; index++) {
            Object.keys(options).forEach(function(key){
                console.log(
                    index % key === 0
                    ? options[key]
                    : index
                );
            });
        }
    }
}
var badgersSongOptions = {
    3: "Badger",
    5: "Mushroom",
    15: "Ooohhh it's a snaaaakkkeeee"
};
var limit = 100;
var BadgersSong = new FizzBuzz(limit, badgersSongOptions);
BadgersSong.startLoop();