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