Skip to main content
Commonmark migration
Source Link

###bool? IsPrime(int n)

bool? IsPrime(int n)

###bool? IsPrime(int n)

bool? IsPrime(int n)

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link
Source Link
Heslacher
  • 50.4k
  • 5
  • 81
  • 175

public static int? LargestPrime(int max)
{
    if (max < 0)
        return null;
    int largestPrime = 0;
    for (int i = 0; i <= max; i++)
    {              
        bool? isPrime = IsPrime(i);
        if ((bool)isPrime)
            largestPrime = i;
        if (i % 100000 == 0)
            Console.WriteLine(largestPrime.ToString("N0"));
    }
    return largestPrime;
}  
  • Omitting braces {}, although they might be optional, won't do you good in the long run because it makes your code error-prone.

  • If an argument of a method isn't correct like max < 0 you should throw either an ArgumentException or better an ArgumentOutOfRangeException.

  • You are iterating from 0 to max to check if the value is a prime, but I wonder what else could the number be? Either it is a prime or it isn't a prime, there is no third way so it doesn't make sense that IsPrime() returns a nullable bool.


###bool? IsPrime(int n)

Here you are iterating from 5 to i*i<n for each number which is passed to the method.

So let us assume that we pass max = 10.000.000 (the dots are there for clearity) this

    if (n % i == 0 || n % (i + 2) == 0)
        return false;

in the for loop in the IsPrime() method is executed 16.194.513 times.

By using a sieve you could pretty much speed this thing up like so

static int CalculateLargestPrime(int maxPrime)
{
    bool[] isComposite = new bool[maxPrime + 1];
    for (int x = 2; x * x <= maxPrime; x++)
    {
        if (!isComposite[x])
        {
            for (int y = x * x; y <= maxPrime; y = y + x)
            {
                isComposite[y] = true;
            }
        }
    }
    for (int i = maxPrime; i >= 0; i--)
    {
        if (!isComposite[i])
        {
            return i;
        }
    }

    return -1;
}  

taken and adjusted from http://codereview.stackexchange.com/a/62158/29371