Smallest Common Multiple – Optimized Advanced Solution

[arcdev1]

Made minor improvements to the advanced solution:

1. The original advanced solution used Math.max and Math.min against the supplied array, this causes the values in the supplied array to be compared twice. If we use sort instead, the values are only compared once and we can target the max and min values using an index. This is more efficient.

2. The original solution caused lcm(min, min + 1) to be evaluated twice. By incrementing min the first time we run lcm we avoid this duplication and also by taking advantage of the prefix increment operator ++min in the while loop, we trim a line.

• I have read freeCodeCamp's contribution guidelines.
• My pull request has a descriptive title (not a vague title like Update index.md)
• My pull request targets the master branch of freeCodeCamp.
• None of my changes are plagiarized from another source without proper attribution.
• My article does not contain shortened URLs or affiliate links.

[ezioda004]

Hi @arcdev1,

Thank you for opening for PR.
Just to clarify, Math.min() and Math.max() both have time complexity of O(n). The complexity of .sort() for small arrays is O(n²) and for big array is O(nlogn), both of which are greater than O(n), so technically using .sort() is slower than using min and max together.

Since we have small arrays in the tests, it doesnt really matter so, using .sort() is fine.

The other content that you've added make the code concise so its fine, but do fix the sort method.

[arcdev1]

Hi @ezioda004, you are right max and min are faster. I mistakenly thought that given the constraints in this very specific case, sort would be faster. But I was wrong.

I recommend we change back to max and min using a spread operator, like so:

let min = Math.min(...arr);
let max = Math.max(...arr);

Here's a working example:
https://repl.it/@arcdev1/Smallest-Common-Multiple

What do you think?

[RandellDawson]

Honestly, for sorting two elements, I would lean towards the sort and do the following which uses destructuring. This increases readability and makes the code more concise at the same time.

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

[RandellDawson]

Also, this is just my opinion, but the Advanced solution probably should use ES6 syntax throughout, so:

function funcName() {

}

becomes:

const funcName = () => {

};

[ezioda004]

@RandellDawson Destructuring is great, but honestly I've seen campers and everyone struggle with it, for the sake of readability I'd avoid that. I like the alt spread syntax, thats ES6 as well.
@arcdev1 I like the spread syntax, the code is more readable and intuitive. I think we should use that.

[RandellDawson]

but honestly I've seen campers and everyone struggle with it, for the sake of readability I'd avoid that.

But this is the advanced solution, where we get to show the campers how to use the advanced features such as destructuring. The spread syntax could be confusing if you have never seen it before also. The destructuring could easily be explained in the explanation of the solution along with links to existing FCC challenges for destructuring arrays.

I believe we need to get some consensus from the senior contributors as to what Advanced means, so we have clearer idea of when we really should be changing these existing working solutions at all. I think the original solution was fine and seems quite readable to me. It could maybe use a few extra bullet points in the explanation regarding the apply method, but otherwise it seems fine to me.

My point here is we need some clearer definitions (which do not currently exist) for when to leave things alone. A thousand people could look at this solution and come up with a "better" solution. We just need to define what makes a beginner, intermediate, and advanced solution different. We should discuss this outside of Github.

[ezioda004]

@RandellDawson There are already existing guidelines. Old solutions had note:

DO NOT add solutions that are similar to any existing solutions. If you think it is similar but better, then try to merge (or replace) the existing similar solution.

So with spread syntax he's merely updating the syntax as spread syntax is quite common these days so updating to newer syntax IMO is fine.

We just need to define what makes a beginner, intermediate, and advanced solution different.

I agree we should discuss this and have a guideline on that but here it's not relevant since the advanced solution already exists and he's simply updating it.

[RandellDawson]

If you think it is similar but better, then try to merge (or replace) the existing similar solution.

The key word is "better". That is subjective and I believe we need at least 2-3 others to agree on if a change makes something "better". Is it better in terms of readability, time complexity, memory complexity, conciseness?

So with spread syntax he's merely updating the syntax as spread syntax is quite common these days so updating to newer syntax IMO is fine.

Destructuring arrays is also quite common these days and is also a newer syntax IMO.

Personally, I like the code written with the apply method, because I actually remember the first time I ever came across the apply method used was from this very Guide article. I exposed me to learn about something totally new and it is not covered in the current curriculum that I am aware of. These older solutions were vetted and reviewed many times over before we got involved. There is a reason why they have not changed, because they are fine the way they are (IMO).

[arcdev1]

My two cents.

@ezioda004 I don't think @RandellDawson 's suggestion violates the following:

DO NOT add solutions that are similar to any existing solutions. If you think it is similar but better, then try to merge (or replace) the existing similar solution.

My interpretation of that guidance is that we should not add a new solution (e.g. an "Advanced Solution 2" alongside the original Advanced Solution). I don't think changing the original one to use destructuring would violate that rule.

I think the trade-offs are:

1. The destructured line is cleaner but less performant and there is a sense that destructuring may be too advanced/confusing for our audience.
2. The min max formulation is more performant and may be more readable for the audience, but may not be as clean or "advanced".

I think given the constraints on the array and the nature of the function, the performance impact is not worth basing our decision on. The question of what is "clean" or "advanced" or even "better" is quite subjective. So, I think we're left with the question of "what is more suitable to our audience, given what we are trying to teach?"

Do we have any empirical evidence we can use to answer that question?

[RandellDawson]

Do we have any empirical evidence we can use to answer that question?

This is why I think we need to get some senior contributors to weigh in on this issue.

The destructured line is cleaner but less performant

Out of curiosity, what makes you believe the destructured line is less performant for a two element array?

[arcdev1]

Out of curiosity, what makes you believe the destructure line is less performant for a two element array?

https://jsperf.com/max-min-vs-sort-lcm

[RandellDawson]

The very fact that we are even debating about this change, means there is a difference in opinion that needs some other way to settle it

https://jsperf.com/max-min-vs-sort-lcm

I have had major issues with that site. I have ran tests on it before and it gives very misleading information and it is easy to break. Plus, what array is being fed into it? I doubt it is a two element array.

[arcdev1]

I have had major issues with that site. I have ran tests on it before and it gives very misleading information. Plus, what array is being fed into it? I doubt it is a two element array.

It is a two element array. I built that in order to formulate a response to a comment from @ezioda004 on the commit. See thread here: #19572 (comment)

My instinct was that sort would be faster in the case of a two element, numeric array. So after @ezioda004 's comment I built that test.

[ezioda004]

@RandellDawson Good points about .apply(). I also learned about that from one of the challenges but by Googling 😄

I have had major issues with that site. I have ran tests on it before and it gives very misleading information. Plus, what array is being fed into it? I doubt it is a two element array.

The array is var arr = [5,1], it's above the "Run Tests" button. The tests are reliant with some very little margin error. Its slow because sort algorithm has overhead. Even though its a small array, it still has overhead nonetheless.

@arcdev1 Good points as well.

[RandellDawson]

Again we are talking about a two element array. A fraction of a fraction of a second difference. When it is that close, I typically go with keeping it concise and the destructured version definitely does that without two lines of Math.min(...arr), Math.max(...arr).

We have different opinions here and because that, we need to table this until we can get others involved. These same kinds of debates occur very often on the forum when someone posts "Is this a better solution than the Guide?". There will be 20 people chime in with their "best" solution. They may all be right. I think all of the solutions presented here are great, but the question is do we really need to change the original solution and I say it is not necessary. That does not mean I think the suggestions made are not good solutions. We just need a formal process of review when it comes to posting "best" solutions in the Guide. I hope you understand what I am getting at here. If we have an agreed upon practice (i.e. 2 out of 3 reviewers say the change is good), then I will be fine even if I am the 1 reviewer who disagreed.

[arcdev1]

do we really need to change the original solution?

I believe the original solution runs lcm(min, min + 1) twice. First outside the while loop and then again as the first iteration of the while loop. If I am right (please sneak a peek) then I think that definitely needs to be corrected.

As for the min max vs sort issue, I think it's down to preference and I agree that if we ask a dozen senior devs we'll get a dozen very good alternatives that may not really be any more effective as a teaching tool than the original.

[RandellDawson]

I believe the original solution runs lcm(min, min + 1) twice. First outside the while loop and then again as the first iteration of the while loop. If I am right (please sneak a peek) then I think that definitely needs to be corrected.

Oh yes, I forgot about that part. Yes, your suggestion is an improvement there for sure.

[arcdev1]

So maybe where we are at is that the ++min change is objectively better and should stay in and that the min max vs sort change is subjective and should just be rolled back to the original. Does that make sense? Do we still need others to weigh in?

[RandellDawson]

After thinking about this a bit more I will go with whatever @ezioda004 thinks. I think this was a good debate and let's us realize we need a formal review process for this kind of thing. I am going to bow out on this one.

[ezioda004]

I'd say updating it with spread syntax is fine. Spread syntax for the most part is syntactic sugar for .apply(null) anyway. One of the reason I'd prefer spread over apply is because .apply() way is hacky. It's not readable at first glance and spread syntax is more used in modern JS objectively.

Sure, you can learn about the behavior of .apply() by googling but syntactic sugar is added to make developers life easier, so why are we shying from using it?

[ezioda004]

@RandellDawson Yes, we should certainly have a discussion about this with higher ups.
Great debate. Talking about performance is always fun for me!

[arcdev1]

Okay. I think we have agreement I will update the code with the spread operator for min max.

FWIW @RandellDawson I really liked your formulation from an aesthetic standpoint and I might have to find a way to use it on something else down the road ;)

[RandellDawson]

Don't forget to convert the helper functions to arrow syntax.

[RandellDawson]

Using destructuring in two places (using your main logic) would yield:

const smallestCommons = arr => {
  const lcm = (a,b) => (a * b) / gcd(a, b);
  const gcd = (a, b) => {
    while (b > 0) {
      [a,b] = [b, a % b];
    }
    return a;
  };
  let [min,max] = arr.sort((a,b)=> a-b);
  let currentLCM = min;
  while (min < max) {
    currentLCM = lcm(currentLCM, ++min);
  }
  return currentLCM;
};

The destructuring also avoids creating a temp variable in the gcd function.

[ezioda004]

I don't mind destructuring for swapping. Also, just remembered the recursive way of GCD

const gcd = (a, b) => b === 0 ? a : gcd(b, a%b);

This is quite famous recursive algorithm.

[arcdev1]

@RandellDawson I think for the top level function, the signature is already there when its presented to the student, so I don't think we should change that to an arrow function. In general, I'm not a fan of arrow functions in these kind of situations, but that's very much a personal preference thing.

Like you though, I'm happy to defer to @ezioda004 on this one.

So, @ezioda004 what say you?

Option A

function smallestCommons (arr) {
  const gcd = (a, b) => b === 0 ? a : gcd(b, a%b);
  const lcm = (a, b) => (a * b) / gcd(a, b);

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

  while (min < max) {
    currentLCM = lcm(currentLCM, ++min);
  }

  return currentLCM;
};

Option B

function smallestCommons (arr) {
  let min = Math.min(...arr)
  let max = Math.max(...arr)
  let currentLCM = min;

  while (min < max) {
    currentLCM = lcm(currentLCM, ++min);
  }

  return currentLCM;
};

function lcm (a, b) { 
  return (a * b) / gcd(a, b);
}

function gcd (a, b) { 
  return b === 0 ? a : gcd(b, a%b);
}

Edit: Moved the helper functions to the global scope in B to stay truer to he original

Or something else?

[ezioda004]

@arcdev1 Option A looks good to me at first glance. Probably because the function expressions are on top 😅
I don't mind B either.
I'll leave it for you guys to decide. (If you still cant decide, flip a coin :D)

BTW great article!