3
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ Welcome to Code Review! I edited your post title and tags because it is extremely similar to the FizzBuzz exercise many programmers do for learning. Hope you get great answers! \$\endgroup\$ – Phrancis Feb 2 '17 at 5:31
3
\$\begingroup\$

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"));
\$\endgroup\$
0
2
\$\begingroup\$
var sequence_function = function (start,limit) {
  var result_array = [];
  start = start || 1;
  limit = limit || 10;

  for(var i = start; i <= limit; i ++) {
    var str='';
    if(!(i%4)) str = 'x';
    if(!(i%7)) str += 'y';
    result_array.push(str || i);
  }
  console.log(result_array.join(','));
  return result_array;
}

Usage:

sequence_function();
// logs out: 1,2,3,x,5,6,y,x,9,10
// returns: [1,2,3,"x",5,6,"y","x",9,10]

Explanation:

Rather than add many conditional cases, we can build up a string for the cases where the i value is divisible.

If the string remains empty we know that all the conditions for divisible tried and we should push a number to the result_array instead.

I use the short-circuit OR binary operator in order to decide what value to push.

\$\endgroup\$
1
  • \$\begingroup\$ @Tushar: thanks, thought it didn't look right! Amended accordingly \$\endgroup\$ – Pineda Feb 2 '17 at 14:13
1
\$\begingroup\$

ES6 solution:

// If using require and have lodash
const { range } = require('lodash');

// If not, define our own. Does not default start=0 but isn't needed in this example
const range = (start, end) => {
  return [...Array(end - start)].map((e, i) => i + start);
};

const sequenceFunction = (start = 1, limit = 10) => {
  // range is exclusive, so need to +1 to include limit
  return range(start, limit + 1).map(i => {
    let representation;

    if (i % 4 === 0) representation = 'x';
    if (i % 7 === 0) representation += 'y';

    return representation || i;
  });
};

// Usage
console.log(sequenceFunction().join(','));

This solution is almost identical to Pineda's and I've provided it just as a syntax comparison.

I agree with the suggested modifications:

  • Parameterising start and limit while still providing default values.
  • Returning the sequence so that it can be used elsewhere if needed.

I have moved the console.log outside of the function so that you can control when it is logged.

Misc

I don't recommend this, it reduces readability. If you wanted map could be reduced to an expression body by abusing String.prototype.repeat() as follows:

const result = range(start, limit + 1).map(i => 
  ('x'.repeat(i % 4 === 0) + 'y'.repeat(i % 7 === 0)) || i
);
\$\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.