Smallest Common Multiple - Where am I messing up?

JavaScript
1

Tell us what’s happening:
I’ve tried to do a good job of explaining my thoughts by commenting in my code below.

My basic process is to generate each multiple of the largest number in the range and then test if that multiple is divisible without a remainder by each of the other numbers in the range. One I get to a number that is I return than number.

I’ve looked over my code and I feel like it should be working, but it clearly isn’t. I don’t think I have any syntax errors. I think the issue must be the order in which I’m calling things.

Would someone mind taking a look at it for me?

Your code so far


function smallestCommons(arr) {
let sorted = arr.sort();//sort to get them arranged lowest to highest.
let lcmArray = [];
//generate all number in the range with a for loop
for (let i = sorted[0]; i <= sorted[1]; i++) {
    lcmArray.push(i);
}


//take a given array as a parameter
function lcmFinder(array) { 
  let test = false;

//set the highest number in the array
  let current = array[array.length -1]; 
  
// use a for loop to generate each multiple of the highest number
  for (let i = 1; test = false; i++) { 
    current *= i;
// creat an array to hold falsey and truthy values; 
    let x = []; 
    
//run another for loop to test if each multiple of the highest number is divisble by all other numbers in the array. 

    for (let j = array[array.length - 2]; j >= array[0]; j--) { 
      if (current%array[j] != 0) { 

//if the remainder doesn't equal 0 push false otherwise push false
        x.push(false);
      } else {
        x.push(true);
      }
    }
// if x does not include any falsey values then set test to true to stop the first for loop and return the value of the current multipe of the largest number in the array.
      if (!x.inludes(false)) { 
        test = true;
        return current;
      }
  }
}



return lcmFinder(lcmArray);
}

smallestCommons([5, 1]);

Your browser information:

User Agent is: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/85.0.4183.83 Safari/537.36.

Challenge: Smallest Common Multiple

Link to the challenge:

2 
// if x does not include any falsey values then set test to true to stop the first for loop and return the value of the current multipe of the largest number in the array.
      if (!x.inludes(false)) { 
        test = true;
        return current;
      }
  • Did you intend for this to be inside the for loop? It is currently after the loop.
  • “inludes” has a typo
  • The value false is not the same as a falsy value. Which are you looking for?
  for (let i = 1; test = false; i++) {

= is an assignment operator, not a comparison operator.

After this I believe that you’ll still find that you are getting incorrect values, but hopefully that should help you move forward on your debugging.

1 Like
3

That was very helpful and exactly the kind of assistance I was looking for. I corrected the issue with the assignment operator because I did intend for it to be a comparison operator, but now I’m getting a warning about an infinite loop :sweat_smile: Do you think I’m going about this process incorrectly?

4

The infinite loop warning is because of the amount of time your code takes to run. To protect your from crashing your browser with an infinite loop. Now that you have a working solution, you can try to make it more efficient.
For this sort of mathy challenge, it’s perfectly valid to look up how to solve the problem mathematically/logically and then figure out how to turn that abstract approach into code.

If you want to look at formulae related to this challenge, the Wikipedia article is good.

5

I’ve been stuck on this same problem for two days now :frowning:

I’ve finally passed all of the tests except for the last two. Any idea why this isn’t working for them?

function smallestCommons(arr) {
  let sorted = arr.sort((a, b) => a - b);
  let rangeArray = [];
   console.log(sorted)
  for (let i = sorted[0]; i <= sorted[1]; i++) {
    rangeArray.push(i);
  }
console.log(rangeArray.length)
let multiplier = 1;
let multiple = (sorted[1] * multiplier);
let loop = 0;
while (loop < 200000) {
  let count = 1;
  for (let i = 1; i < rangeArray.length; i++) {
    if (multiple % rangeArray[i] === 0) {
        count += 1;      
        console.log(count, rangeArray[i], multiple)
    }     
    if (count === rangeArray.length) {
      return multiple
    }
      
      
      //console.log(count + "log")
  }  
  loop++;
  multiple = (multiplier += 1) * sorted[1];
  
  
}

}

smallestCommons([1, 13]);
6

It’s probably timing out on those.

Why do you have this loop though?

while (loop < 200000) {
1 Like
7

The whole while loop or the loop variable?

I have the while loop to increment to the next multiple of the largest number in the range. The loop variable serves to give it an ending and prevent an infinite loop. I wasn’t sure how else to solve the infinite loop warning.

8

You are getting the infinite loop warning because your code takes too long.

In this situation, I ask myself

  1. What calculations am I doing that are not needed

  2. What am I storing that I only use once?

For example,

Why not stop checking as soon as you find a number in the range that isn’t a divisor?

Why check sorted[1] when you are already using multiples of sorted[1] as your guesses?

Logging to the console is very slow.

Why not just add sorted[1] here?

3 Likes
9

Thank you your suggestions helped me break out of my fog and improve the speed of my calculations!

Here is my solution that passed:

function smallestCommons(arr) {
  let sorted = arr.sort((a, b) => a - b);
  let rangeArray = [];
   
  for (let i = sorted[0]; i <= sorted[1]; i++) {
    rangeArray.push(i);
  }
let secondLargest = rangeArray[rangeArray.length - 2];
let multiplier = 1;

let count= 1;
let loop = 0;
while (loop < 50000) {
  let multiple = (sorted[1] * secondLargest) * multiplier;
  for (let i = 0; i < rangeArray.length; i++) {
    if (multiple % rangeArray[i] !== 0) {     
         break;
    } else  {
      count++;
    } 
    if (count === rangeArray.length) {
      return multiple
    }
    }       
  loop++;
  multiplier += 1;
  count = 1;
}

}

smallestCommons([1, 5]);
10

@JeremyLT is a former math teacher and a current developer for high performance computing, so he’s always a good source of advice on these challenges. :smiley:

2 Likes