HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Badger's FizzBuzz

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
badgerfizzbuzzstackoverflow

Problem

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?

Solution

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

Code Snippets

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

Context

StackExchange Code Review Q#113488, answer score: 5

Revisions (0)

No revisions yet.