Not sure why this doesn't pass

Tell us what’s happening:
Everyone has tick boxes other than the last any idea why?

  **Your code so far**

function smallestCommons(arr) {
const [min, max] = arr.sort((a, b) => a - b);
console.log(min);
let numarray = makearray(min, max);
let maxx = max;
let count = 0;
while (true){
  for (let index = 1; numarray.length > index; ++index) {
    if (maxx % numarray[index] !== 0){     
      maxx++;
      count = 0; 
}
else {
  count = count +1;
  if (count === numarray.length){
      return maxx;
  }}}}}
function makearray (min, max){
let numbers = [];
for (let i = min; i <= max; i++){
  numbers.push(i);
}
return numbers
}


smallestCommons([23, 18]);
  **Your browser information:**

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

Challenge: Smallest Common Multiple

Link to the challenge:

  1. You need to format your code better to show the structure clearly. I need to reformat you code to read it. Poorly formatted code makes it really hard to read and difficult to understand. This results in difficult to detect errors creeping in.

  2. Avoid overkill solution. Make the code clean and simple. Don’t obscure the code.

const [min, max] = arr.sort((a, b) => a - b);

arr has only two elements always. By sorting an array, the first impression the reader gets is you’re trying to sort an array. But that is not what you really need to do. The objective is to set the minimum and maximum of the passed two values. Simple if will tell the story much clearer.

let [min, max] = arr
if (min > max) [max, min] = [min, max]

Or

if (arr[0] < arr[1]) {
  [min, max] = arr
} else {
  [max, min] = arr
  1. The main loop is awkward. You have the top level while true loop. You set the condition to true, so it is an infinite loop. Which means there must be a test condition inside the while loop to exit the while. That condition is nested inside the for-loop. You want to include a test at the top level, not nested inside another loop to make the logic of the loop clear.

But what is the purpose of this while loop? You want to exit this loop when you found the solution. So, what you have is

let isFound = false
while (!isFound) {
   if (all values from min to max divide smallestCommonMultiple) {
     isFound = true
  } else {
    update smallestCommonMultiple
  }
}
return smallestCommonMultiple

You can define a Boolean function divideAll(min, max, smallestCommonMultiple). It returns true if all values from min to max divide smallestCommonMultiple. Otherwise it returns false.

  1. You’re creating an array from min and max. Why? Is it really necessary? You’re simply counting from min and max. This is related to Item 2. Avoid an overkill solution.

  2. When this test

if (maxx % numarray[index] !== 0){     
        maxx++;
        count = 0; 
      }

is true, you want to break out from the for loop and continue with next maxx. Even though not necessary, you continue the loop till the end. Because you’re testing count === numbers.length, it will work, but this is not a good approach and is error-prone.

  1. Finally, why your code is not working. It is actually a simple fix. But it took a bit to find out the error, because it took me time to understand your code. This is the reason why I gave a long reply about the necessity of clean and nicely structured code.
    Change your for loop to
for (let index = 0;  index < numarray.length; ++index) {

and see if this will pass the tests. I think it will. But, do give some thoughts on writing a “logically clear” code.

1 Like

Thank you for the detailed and clear answer. I will take my time going through it and improving my skills.