I saw it would be better if I post an answer instead of using comments.
For your IsPrime
method, I see that you've covered most conditions, but you forgot to cover 0, 1, and -n cases, which you can just do with a small change on this line :
if (n % 2 == 0) return false;
to
if (n < 2 || n % 2 == 0) return false;
suggested :
public static bool IsPrime(int n)
{
if(n == 2) { return true; }
if (n < 2 || (n % 2 == 0)) { return false; }
for (int x = 3; x * x <= n; x += 2)
{
if (n % x == 0) { return false; }
}
return true;
}
For GetDivisors
. I assume that you excluded 1
and n
from the results, since it's already known that every natural number is divisible by 1 and itself, which is fine if you intended to use this code personally, but it is uncommon to do that, and it might even conflict with other developers code, as you don't want to assume everybody knows that! so it must be included to make the code more usable for others, and you must always consider what is common use, and what is not.
The technique in @gazoh answer is a really good one, and I would take it to the next level, but I have to note out that Sort
and ToArray
are expensive operations as I've mentioned in the comments. And I would avoid using them directly.
I've updated this method to :
private static IEnumerable<int> GetDivisors(int n)
{
if (n <= 0) { yield return default; }
int iterator = (int)Math.Sqrt(n);
for (int i = 1; i <= iterator; i++)
{
if (n % i == 0)
{
yield return i;
if (i != n / i) { yield return n / i; }
}
}
}
public static IEnumerable<int> GetDivisors(int n, bool AscendingOrder = false)
{
return !AscendingOrder ? GetDivisors(n) : GetDivisors(n).OrderBy(x => x);
}
I've used IEnumerable<int>
to open more acceptable collections types rather than just sticking with an array. The overload is just to have an option to order the data, while the default is unordered
data. This would make it optional, which would depend on usage, if you prefer performance over order, or order over performance. Then you can convert it to list or array or any collection.
For performance differences, I've used BenchmarkDotNet
to test and compare this method performance and here is the results.
Test 1 using GetDivisors(2095133040)
| Method | Mean | Error | StdDev |
|------------------- |---------------------:|--------------------:|--------------------:|
| Original | 5,907,229,864.000 ns | 116,153,274.6298 ns | 155,061,285.3568 ns |
| ByGazoh | 169,411.783 ns | 2,805.6563 ns | 2,487.1413 ns |
| IEnumerable<int> | 6.637 ns | 0.1896 ns | 0.2107 ns |
| OrderBy | 17.514 ns | 0.3013 ns | 0.2516 ns |
| ToArray | 151,408.141 ns | 2,906.1875 ns | 2,854.2648 ns |
| ToList | 363,424.079 ns | 5,318.8335 ns | 4,975.2401 ns |
| ToArray + OrderBy | 154,249.309 ns | 2,370.8673 ns | 2,101.7121 ns |
| ToList + OrderBy | 356,705.127 ns | 6,002.7773 ns | 5,321.3057 ns |
Test 2 using GetDivisors(1600)
| Method | Mean | Error | StdDev |
|------------------- |-------------:|-----------:|-----------:|
| Original | 4,804.474 ns | 93.0708 ns | 91.4080 ns |
| ByGazoh | 515.822 ns | 7.6545 ns | 7.1601 ns |
| IEnumerable<int> | 6.391 ns | 0.0966 ns | 0.0904 ns |
| OrderBy | 16.783 ns | 0.2839 ns | 0.2517 ns |
| ToArray | 422.570 ns | 6.3368 ns | 5.9274 ns |
| ToList | 463.575 ns | 8.0975 ns | 7.5744 ns |
| ToArray + OrderBy | 1,662.728 ns | 26.8204 ns | 25.0878 ns |
| ToList + OrderBy | 1,634.595 ns | 30.9492 ns | 28.9499 ns |
ns = nano-second;
Where
OrderBy
= Divisors.GetDivisors(n).OrderBy(x=>x);
ToArray
= Divisors.GetDivisors(n).ToArray();
ToList
= Divisors.GetDivisors(n).ToList();
ToArray + OrderBy
= Divisors.GetDivisors(n).OrderBy(x=>x).ToArray();
ToList + OrderBy
=Divisors.GetDivisors(n).OrderBy(x=>x).ToList();
Lastly, regarding your tests, I suggest you test each scenario separately.
Example :
[TestMethod]
public void GetDivisors_15_IsEqual()
{
Assert.AreEqual(new int[] { 3, 5 }, Divisors.Divisors(15));
}
[TestMethod]
public void GetDivisors_16_IsEqual()
{
Assert.AreEqual(new int[] { 2, 4, 8 }, Divisors.Divisors(16));
}
[TestMethod]
public void GetDivisors_253_IsEqual()
{
Assert.AreEqual(new int[] { 11, 23 }, Divisors.Divisors(253));
}
[TestMethod]
public void GetDivisors_24_IsEqual()
{
Assert.AreEqual(new int[] { 2, 3, 4, 6, 8, 12 }, Divisors.Divisors(24));
}
the reason is simple, if you create a separate test for each scenario you have, it'll be easy to determine which part of your code needs adjustments, and it would make things easier for improving your code (say you want to simplify it without breaking the code). Also, it would be more easier to read and follow, and could give you a better view on the requirements, and the validation process of it.
If you don't detailed your tests, in smaller projects you might not have any issues, but in big projects, it'll be a pain in the neck.
I hope this would be useful.
c#
language tag. \$\endgroup\$Divisors.Divisors
a typo? \$\endgroup\$