Smallest Common Multiple - please review my code!

I finally solved the smallest common multiples challenge after a bit more than an hour, with the initial setup from yesterday of just the â€śbetweenâ€ť array. It seems like a really long solution, but I also used the code from previous challenge (Summing All Primes) and I had to build additional function to solve the real challenge.

I would really appreciate any feedback! Donâ€™t mind long commenting on the code, I was too excited I am doing this in around three weeks!

``````function smallestCommons(arr) {
var num1 = arr[0], num2 = arr[1], between = [], all = [], mult = [], res = 1;

// Creating an array of range between the numbers given
if (num1 < num2) {
for (var i = num1+1; i < num2; i++) {
between.push(i);
}
} else if (num1 > num2) {
for (var j = num1-1; j > num2; j--) {
between.push(j);
}
}

// Creating a decending array of numbers given + the range in between
between.unshift(num1);
between.push(num2);
all = between.sort(function(a,b) {

return b - a;
});

// Additiaonl function for checking if any element in arr (first argument) is divisible by divisor (second argument). God, I sound like FCC instructions.
function ifAnyDivisibleBySth(arr, divisor) {
var result;
for (var k = 0; k < arr.length; k++) {
if (arr[k] % divisor === 0) {
result = true;
break;
} else {
result = false;
}
}
return result;
}

// Function for getting primes, from previous challenge. Only this code is borrowed.
function getPrimes(max) {
var sieve = [], x, y, primes = [];
for (x = 2; x <= max; ++x) {
if (!sieve[x]) {
// x has not been marked -- it is prime
primes.push(x);
for (y = x << 1; y <= max; y += x) {
sieve[y] = true;
}
}
}
return primes;
}

// getting the necessary primes
var primesArray = getPrimes(all[0]);

// I actually built this stuff. I cannot believe this LOL
// To clear the probable confusion: We got all the primes in
// primesArray. We go through each prime (first for loop) and
// divide each element of "all" array with each prime as long
// as it can be divided evenly (while loop) and count the
// common multiples by adding them to "mult" array (second for loop).
for (prime = 0; prime < primesArray.length; prime++) {
while (ifAnyDivisibleBySth(all, primesArray[prime])) {
for (var n = 0; n < all.length; n++) {
if (all[n] % primesArray[prime] === 0) {
all[n] /= primesArray[prime];
}
}
mult.push(primesArray[prime]);
}
}

// When we counted the multiples correctly, it's easy. We just do a multiplication of all of them to get the least common multiple.
// PS Easy? This for loop was so complicated for me just two weeks ago! LOL
for (var f = 0; f < mult.length; f++) {
res *= mult[f];
}

return res;
}

``````

To make the first part (create an array with all numbers in the range) shorter, you can sort the array so `num1 < num2` will always be true and you will just need the first for loop. There are a lot of ways to do this, like `arr.sort(...)` or `Math.max()` and `Math.min()`â€¦ A lot

You can let `num1` be the smaller of the two inputs and `num2` be the larger one

``````  var num1 = Math.min(arr[0], arr[1]),
num2 = Math.max(arr[0], arr[1]),
``````

With this you can eliminate the if-else and just have one for-loop.

``````for (var i = num2; i >= num1; i--) {
between.push(i);
}
// also takes care of the endpoints, so `unshift` and `push` after the loop are not needed
``````

`i` is decreasing so you donâ€™t have to sort in reverse order. You can also forget about `all` and just use `between`, since all `all = between.sort();` does is make `all` point to the same array that `between` points to.

The loop in `ifAnyDivisibleBySth` can be simplified by returning early if `arr[k] % divisor === 0`.

``````  function ifAnyDivisibleBySth(arr, divisor) {
for (var k = 0; k < arr.length; k++) {
if (arr[k] % divisor === 0) {
return true;
}
}
return false;
}
``````

When I only have one if-block in a loop with no else-partner, I use `continue` so my code is less deep.

``````for (x = 2; x <= max; ++x) {
if (sieve[x]) continue;

// x has not been marked -- it is prime
primes.push(x);
for (y = x << 1; y <= max; y += x) {
sieve[y] = true;
}
}
``````

You can use `.reduce` to collect the values in `mult` into a single product (and ditch the for-loop and `res`).

``````return mult.reduce((prev, current) => prev * current, 1);
``````

Iâ€™d advice against giving the `prime` variable the name `prime` because itâ€™s actually an index to a prime. Iâ€™d call it `primeIndex`. But thatâ€™s just me.

This is my solution if you're interested. It doesn't look for primes and relies on the fact that least common multiple is associative (that is, lcm(a, b, c) == lcm(a, lcm(b, c)) and so on, and getting the LCM of two numbers is relatively easy)
``````
// precondition: arr is sorted
function expandArr(arr) {
var nums = [];
for (var n = arr[0]; n <= arr[1]; n++) {
nums.push(n);
}
return nums;
}

function lcm(a, b) {
if (Math.max(a, b) % Math.min(a, b) === 0) {
return Math.max(a, b);
}

var result = a * b;
for (var fi = Math.min(a, b) - 1; fi > 1; fi--) {
if (fi === Math.min(a, b)) {
continue;
}

if (a % fi === 0 && b % fi === 0) {
result /= fi;
a /= fi;
b /= fi;
}
}
return result;
}

function smallestCommons(arr) {
arr.sort(function(a, b) { return a - b; });
var nums = expandArr(arr);

return nums.reduce(lcm);
}
``````
1 Like

Thanks @Atomk @kevcomedia Those are nice tips!

I would really like to hear from you as well @P1xt