Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

#Comments are distracting#

Comments are distracting

The number of comments in the code is too much and distracts from reading the code. I would prefer one large comment before each function explaining things rather than one comment per line.

#Inefficiencies#

Inefficiencies

There are some places in the code that are inefficient:

  1. When you find a prime factor, you set i = 1. You don't need to do that because you won't find any more prime factors less than i. What you do need to do is keep retrying i because the same prime factor can divide multiple times into your number.

  2. After each division, you check if the remaining number is a prime. I'm not sure if that makes things faster or slower, but if number is prime, you should return immediately instead of continuing in the loop, because there will be no more divisors.

#Faster method#

Faster method

A faster way of doing this would be to precompute the primes from 1..N using something like a Sieve of Eratosthenes algorithm. That way, your isPrime() function becomes an array lookup instead of a loop. Also, if you generate a list of primes from the Sieve, your main factoring loop will be much faster because you can only divide by primes instead of dividing by every number.

I wrote a program using this method and it was 50x faster than your program when computing up to 100000, and 600x faster than your program when computing up to 1000000 (0.60 seconds vs 365 seconds).

#Comments are distracting#

The number of comments in the code is too much and distracts from reading the code. I would prefer one large comment before each function explaining things rather than one comment per line.

#Inefficiencies#

There are some places in the code that are inefficient:

  1. When you find a prime factor, you set i = 1. You don't need to do that because you won't find any more prime factors less than i. What you do need to do is keep retrying i because the same prime factor can divide multiple times into your number.

  2. After each division, you check if the remaining number is a prime. I'm not sure if that makes things faster or slower, but if number is prime, you should return immediately instead of continuing in the loop, because there will be no more divisors.

#Faster method#

A faster way of doing this would be to precompute the primes from 1..N using something like a Sieve of Eratosthenes algorithm. That way, your isPrime() function becomes an array lookup instead of a loop. Also, if you generate a list of primes from the Sieve, your main factoring loop will be much faster because you can only divide by primes instead of dividing by every number.

I wrote a program using this method and it was 50x faster than your program when computing up to 100000, and 600x faster than your program when computing up to 1000000 (0.60 seconds vs 365 seconds).

Comments are distracting

The number of comments in the code is too much and distracts from reading the code. I would prefer one large comment before each function explaining things rather than one comment per line.

Inefficiencies

There are some places in the code that are inefficient:

  1. When you find a prime factor, you set i = 1. You don't need to do that because you won't find any more prime factors less than i. What you do need to do is keep retrying i because the same prime factor can divide multiple times into your number.

  2. After each division, you check if the remaining number is a prime. I'm not sure if that makes things faster or slower, but if number is prime, you should return immediately instead of continuing in the loop, because there will be no more divisors.

Faster method

A faster way of doing this would be to precompute the primes from 1..N using something like a Sieve of Eratosthenes algorithm. That way, your isPrime() function becomes an array lookup instead of a loop. Also, if you generate a list of primes from the Sieve, your main factoring loop will be much faster because you can only divide by primes instead of dividing by every number.

I wrote a program using this method and it was 50x faster than your program when computing up to 100000, and 600x faster than your program when computing up to 1000000 (0.60 seconds vs 365 seconds).

Corrected speed factor of sieve program after fixing a bug in it.
Source Link
JS1
  • 28.9k
  • 3
  • 41
  • 83

#Comments are distracting#

The number of comments in the code is too much and distracts from reading the code. I would prefer one large comment before each function explaining things rather than one comment per line.

#Inefficiencies#

There are some places in the code that are inefficient:

  1. When you find a prime factor, you set i = 1. You don't need to do that because you won't find any more prime factors less than i. What you do need to do is keep retrying i because the same prime factor can divide multiple times into your number.

  2. After each division, you check if the remaining number is a prime. I'm not sure if that makes things faster or slower, but if number is prime, you should return immediately instead of continuing in the loop, because there will be no more divisors.

#Faster method#

A faster way of doing this would be to precompute the primes from 1..N using something like a Sieve of Eratosthenes algorithm. That way, your isPrime() function becomes an array lookup instead of a loop. Your Also, if you generate a list of primes from the Sieve, your main factoring loop will also benefitbe much faster because you can only try dividingdivide by primes instead of dividing by every number.

I wrote a program using this method and it was about 7x50x faster than your program when computing up to 100000, and 600x faster than your program when computing up to 1000000 (0.60 seconds vs 365 seconds).

#Comments are distracting#

The number of comments in the code is too much and distracts from reading the code. I would prefer one large comment before each function explaining things rather than one comment per line.

#Inefficiencies#

There are some places in the code that are inefficient:

  1. When you find a prime factor, you set i = 1. You don't need to do that because you won't find any more prime factors less than i. What you do need to do is keep retrying i because the same prime factor can divide multiple times into your number.

  2. After each division, you check if the remaining number is a prime. I'm not sure if that makes things faster or slower, but if number is prime, you should return immediately instead of continuing in the loop, because there will be no more divisors.

#Faster method#

A faster way of doing this would be to precompute the primes from 1..N using something like a Sieve of Eratosthenes algorithm. That way, your isPrime() function becomes an array lookup instead of a loop. Your main factoring loop will also benefit because you can only try dividing by primes instead of by every number.

I wrote a program using this method and it was about 7x faster than your program when computing up to 100000.

#Comments are distracting#

The number of comments in the code is too much and distracts from reading the code. I would prefer one large comment before each function explaining things rather than one comment per line.

#Inefficiencies#

There are some places in the code that are inefficient:

  1. When you find a prime factor, you set i = 1. You don't need to do that because you won't find any more prime factors less than i. What you do need to do is keep retrying i because the same prime factor can divide multiple times into your number.

  2. After each division, you check if the remaining number is a prime. I'm not sure if that makes things faster or slower, but if number is prime, you should return immediately instead of continuing in the loop, because there will be no more divisors.

#Faster method#

A faster way of doing this would be to precompute the primes from 1..N using something like a Sieve of Eratosthenes algorithm. That way, your isPrime() function becomes an array lookup instead of a loop. Also, if you generate a list of primes from the Sieve, your main factoring loop will be much faster because you can only divide by primes instead of dividing by every number.

I wrote a program using this method and it was 50x faster than your program when computing up to 100000, and 600x faster than your program when computing up to 1000000 (0.60 seconds vs 365 seconds).

Source Link
JS1
  • 28.9k
  • 3
  • 41
  • 83

#Comments are distracting#

The number of comments in the code is too much and distracts from reading the code. I would prefer one large comment before each function explaining things rather than one comment per line.

#Inefficiencies#

There are some places in the code that are inefficient:

  1. When you find a prime factor, you set i = 1. You don't need to do that because you won't find any more prime factors less than i. What you do need to do is keep retrying i because the same prime factor can divide multiple times into your number.

  2. After each division, you check if the remaining number is a prime. I'm not sure if that makes things faster or slower, but if number is prime, you should return immediately instead of continuing in the loop, because there will be no more divisors.

#Faster method#

A faster way of doing this would be to precompute the primes from 1..N using something like a Sieve of Eratosthenes algorithm. That way, your isPrime() function becomes an array lookup instead of a loop. Your main factoring loop will also benefit because you can only try dividing by primes instead of by every number.

I wrote a program using this method and it was about 7x faster than your program when computing up to 100000.

lang-cpp

AltStyle によって変換されたページ (->オリジナル) /