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

Sequence program (FizzBuzz-like)

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

Problem

I have wrote a program which prints sequence of numbers. But if number is divisible by 4 it prints x if divisible by 7 it prints y, if divisible by both 4 & 7 it prints xy otherwise it prints number.

var sequence_function = function () {
    var result_array = [],
        start = 1,
        limit = 10;

    for(i = start; i <= limit; i ++) { 
        switch (true) {
            case (i % 4 === 0 && i % 7 === 0):
                result_array.push('xy');
                break;
            case (i % 4 === 0):
                result_array.push('x');
                break;
            case (i % 7 === 0):
                result_array.push('y');
                break;
            default:
                result_array.push(i);
                break;
        }
    }

    console.log(result_array.join(','));
};


So for limit 10 it gives output:

1,2,3,x,5,6,y,x,9,10


My question if how can i optimise this code? What is best thing to use here if else, switch, ternary operator or any other? Any suggestion will be helpful.

Solution

Naming

sequence_function is not a very good name for this function. For one, we already know it's a function, no need to name as such (you don't name your variables foo_variable right?). Likewise, result_array can just be results.

Second, what does "sequence" mean in context? When you look at the code that uses this function again in 6 months, will you be able to understand what this sequence_function is for without having to go back and read the code of the function? Good naming is one of the most important aspects of maintainable code.

Let's name it multipleOf4Or7 for now. Note that camelCase naming is usually used for JS, rather than snake_case.

Conditionals

Your conditionals would read better using plain if/else if/else in my opinion. If you're using something like switch(true) it's probably a sign that it's not the best structure. Using regular conditionals, you can also reduce one conditional branch, and the code is a lot shorter.

So far...

function multipleOf4Or7() {
    var results = [],
        start = 1,
        limit = 10;

    for(i = start; i <= limit; i ++) {
        if (i % 4 === 0) {
            if (i % 7 === 0) {
                results.push('xy');
            } else {
                results.push('x');
            }
        } else if (i % 7 === 0) {
            results.push('y');
        } else {
            results.push(i);
        }
    }
    console.log(results.join(','));
};


Return from function

You ought to return the results array from the function, instead of making the function print it. The advantage of that is if you need to do something else with the result, you can just let the caller handle it and not have to modify the function. Just replace console.log(results.join(',')); with return results, then just call like this:

console.log(multipleOf4Or7().join(','));


Hard-coded values vs. Default values

Right now your function is not very flexible, because all the values are hard-coded into the function.

-
What if you wanted to run it for 30 instead of 10?

-
What if you wanted to use 3 and 5, instead of 4 and 7?

-
What if you wanted to use values other than x and y?

Right now, you can't do any of that without changing or copying the function. JavaScript functions can have default parameters which would make it to where regular function calls with no arguments will return the exact same results, but you do have the option to call it with different arguments.

To do that, let's move all the hard-coded values into named parameters in the function signature of a new function, and replace the hard coded values:

function multipleOfOneOrBoth(num1 = 4, num2 = 7,
                             val1 = 'x', val2 = 'y',
                             start = 1, limit = 10) {
    var results = [];
    for(i = start; i <= limit; i ++) {
        if (i % num1 === 0) {
            if (i % num2 === 0) {
                results.push(val2);
            } else {
                results.push(val1);
            }
        } else if (i % num2 === 0) {
            results.push(val2);
        } else {
            results.push(i);
        }
    }
    return results;
}


Then simply replace the existing function's body like this:

function multipleOf4Or7() {
    return multipleOfOneOrBoth();
}


You could also, instead of using default arguments, just write

function multipleOf4Or7(start = 1, limit = 10) { 
    return multipleOfOneOrBoth(4, 7, 'x', 'y', start, limit); 
}


With this you can easily use it for other things, for example a FizzBuzz :

console.log(multipleOfOneOrBoth(3,5,"Fizz","Buzz",1,30).join("\n"));

Code Snippets

function multipleOf4Or7() {
    var results = [],
        start = 1,
        limit = 10;

    for(i = start; i <= limit; i ++) {
        if (i % 4 === 0) {
            if (i % 7 === 0) {
                results.push('xy');
            } else {
                results.push('x');
            }
        } else if (i % 7 === 0) {
            results.push('y');
        } else {
            results.push(i);
        }
    }
    console.log(results.join(','));
};
console.log(multipleOf4Or7().join(','));
function multipleOfOneOrBoth(num1 = 4, num2 = 7,
                             val1 = 'x', val2 = 'y',
                             start = 1, limit = 10) {
    var results = [];
    for(i = start; i <= limit; i ++) {
        if (i % num1 === 0) {
            if (i % num2 === 0) {
                results.push(val2);
            } else {
                results.push(val1);
            }
        } else if (i % num2 === 0) {
            results.push(val2);
        } else {
            results.push(i);
        }
    }
    return results;
}
function multipleOf4Or7() {
    return multipleOfOneOrBoth();
}
function multipleOf4Or7(start = 1, limit = 10) { 
    return multipleOfOneOrBoth(4, 7, 'x', 'y', start, limit); 
}

Context

StackExchange Code Review Q#154221, answer score: 3

Revisions (0)

No revisions yet.