feat: add prime sieve and generator#50
Conversation
|
@appgurueu What about this PR? Seems like it's finished 🤔 |
appgurueu
left a comment
appgurueu
left a comment
There was a problem hiding this comment.
Looks like I forgot to submit my pending review 😅
| * @returns All prime numbers from 2 through {@linkcode limit} | ||
| */ | ||
| export function sieveOfEratosthenes(limit: number): number[] { | ||
| assert(limit > 1, "limit should be an integer greater than 1"); |
There was a problem hiding this comment.
Suggestion: Replace all of this - including the assert def. - with if (!Number.isInteger(limit) || limit <= 1) throw new Error("limit should be an integer greater than 1");.
|
|
||
| return maybePrime | ||
| .reduce( | ||
| (primes, isPrime, number) => (isPrime ? [...primes, number] : primes), |
There was a problem hiding this comment.
This seems horribly inefficient (and also unreadable) to me: It always copies the entire primes array so far just to append a single value. This implies quadratic runtime in the number of primes. Please write it out imperatively:
const primes = [];
for (let i = 2; i < primes.length; i++) if (maybePrime[i]) primes.push(i);which seems more concise, performant, and readable to me.
There was a problem hiding this comment.
I'm currently finishing this PR. There is a little spelling mistake in your suggestion. In the condition of the for-loop you typed primes instead of maybePrime :)
There was a problem hiding this comment.
I'm currently finishing this PR. There is a little spelling mistake in your suggestion. In the condition of the for-loop you typed
primesinstead ofmaybePrime:)
Indeed, that ought to be maybePrime.
Happens ;) If you have the time, maybe you could go through the other PRs aswell. I think #97 has a Review pending too and maybe some can be sorted out. If people are no longer working on it, I will try Finishing their PRs. |
|
Finished this PR in #144 |
3 participants
No description provided.