 # 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, num2 = arr, 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);

// 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, arr),
num2 = Math.max(arr, arr),
``````

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; n <= arr; 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 