Optimised palindrome-products solution #1398
Conversation
|
(@junedev added you for your eyes. @joshgoebel I know you find these really interesting) |
|
How so? Passes ALL tests here locally. |
|
@surya-11dev, that's a different issue. That's related to #898, and the test-runner trying to run the skipped test. |
Thank u very much sir for making time to explain things. |
sorry sir, nothing but some misunderstanding. Thanks for the explanation. Kind of u. |
|
@SleeplessByte Thanks for pinging me, I was mentoring this student so I had seen this early! |
|
@joshgoebel @SleeplessByte Could you two decide whether this can be merged please? I am not very familiar with this exercise. |
| @@ -79,7 +79,7 @@ describe('Palindromes', () => { | |||
| expect(sortFactors(smallest.factors)).toEqual(expected.factors); | |||
| }); | |||
|
|
|||
| test.skip('largest palindrome from four digit factors', () => { | |||
| xtest('largest palindrome from four digit factors', () => { | |||
There was a problem hiding this comment.
Pretty sure this was intentional.
There was a problem hiding this comment.
I think the previous solution caused some error when ran into this test case, so the maintainer skipped this test by test.skip. After the changes, it should work out fine, so I reverted test.skip to xtest.
There was a problem hiding this comment.
This is on purpose the test runner on the website is not supposed to run this because it can take too long. There is a bug now where instead it fails. That's a separate issue.
| @@ -70,14 +68,22 @@ export class Palindromes { | |||
| } | |||
|
|
|||
| get smallest() { | |||
| for (let m = this.minFactor; m <= this.maxFactor; m += 1) { | |||
There was a problem hiding this comment.
Isn't this the same exact algorithm just written in a much more complex manner? Isn't the loop still covering exactly the same territory? Since you're just finishing the first item you can't get the same benefits you could by optimization the max loop.
There was a problem hiding this comment.
Please see #1407. I have updated the code after our discussion on exercism.io
There was a problem hiding this comment.
I'll repeat: How is the behavior of this code any different than the previous code? (in regards to smallest) Please explain? From what I can see the for loops do the exact same thing - cover the exact same ground, etc, but they were clearer before with nested for loops.
There was a problem hiding this comment.
The difference is that they don't generate the "same" pairs. For instance, the for loop solution will generate both 3x5 and 5x3. In the updated solution, there will only be 3x5.
There was a problem hiding this comment.
Can't this be resolved by simply having the second for loop start with m instead of minFactor, avoiding looping over the same territory as prior passes?
There was a problem hiding this comment.
Yes, for loops can achieve the same. I was following the logic and coding style I had in the get largest
There was a problem hiding this comment.
Adding complexity solely for symmetry is not a win IMHO. If the simpler for loop solution work for minimum, lets not complicate it.
There was a problem hiding this comment.
Thanks for the comment. I have updated the code accordingly to the above two points in #1407. The reason I started another pull request is that I realized I mixed a few other previous merged commits in this pull request, and I wasn't sure if that would make the process more complicated when you guys merge the changes
|
Closing this as it is replaced by #1407 |
Hi, the original solution takes around 30s to pass all the tests on my machine. I have greatly optimized the solution and it only takes 1.5s to pass all the tests now.