patternjavascriptMinor
Sequence program (FizzBuzz-like)
Viewed 0 times
programlikefizzbuzzsequence
Problem
I have wrote a program which prints sequence of numbers. But if number is divisible by 4 it prints
So for limit
My question if how can i optimise this code? What is best thing to use here
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,10My 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
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
Let's name it
Conditionals
Your conditionals would read better using plain
So far...
Return from function
You ought to return the
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
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:
Then simply replace the existing function's body like this:
You could also, instead of using default arguments, just write
With this you can easily use it for other things, for example a FizzBuzz :
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.