Smallest Common Multiple - please review my code!

1

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;
}
2

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 :slight_smile:

3

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
4

Thanks @Atomk @kevcomedia :slight_smile: Those are nice tips!

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