Skip to content
This repository was archived by the owner on Aug 1, 2021. It is now read-only.

Fix #540 : Added more tests to nth-prime and altered example code - #542

Merged
matthewmorgan merged 1 commit into
exercism:masterfrom
gavinhenderson:master
Jun 14, 2018
Merged

matthewmorgan merged 1 commit into
exercism:masterfrom
gavinhenderson:master

Conversation

@gavinhenderson

Copy link
Copy Markdown
Contributor

The test execute time has gone from 2.975s with the old example code and the old tests to 3.200s with the new tests and the new example code.

@matthewmorgan

matthewmorgan commented Jun 12, 2018 •

Copy link
Copy Markdown
Contributor

@gavinhenderson looks great. I took a similar approach with my solution a while back! Though you can see I was lazy and imported my prior sieve function...

I appreciate the fact that you commented explaining the magical math. It might be improved slightly by referring to a citation that explains the formula. By way of example, here's that old solution I mentioned:

import sieve from './sieve';  //my Sieve class from earlier

export default class Prime{

  // see http://en.wikipedia.org/wiki/Prime_number_theorem#Approximations_for_the_nth_prime_number
  nth(n) {
    if (n < 1) throw new Error("Prime is not possible");
    let approximateLimit = n * Math.log(n) + n * Math.log(Math.log(n));
    approximateLimit < 15 ? approximateLimit = 15 : {};
    return sieve(approximateLimit).primes[n - 1];
  }
}

Would you mind making that comment change? Otherwise this PR looks good to me.

@gavinhenderson

Copy link
Copy Markdown
Contributor Author

@matthewmorgan Thats the link added now

@matthewmorgan

Copy link
Copy Markdown
Contributor

@gavinhenderson awesome-- thank you.

Can I ask you to squash your commits to one for this PR? Please let me know if you need assistance with this, I'd be happy to help.

@gavinhenderson

Copy link
Copy Markdown
Contributor Author

@matthewmorgan So I think I might have done it?

@matthewmorgan

Copy link
Copy Markdown
Contributor

Nicely done. Thanks for your contribution!

@matthewmorgan
matthewmorgan merged commit 12cf744 into exercism:master Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants