How to optimize this c# code?

Question

I have a challenge on a programming platform (CodeWars - "Find the divisors") and my algorithm seems to be too slowly. This is error which I get from platform: Process was terminated. It took longer than 12000ms to complete

This is instructions for challenge: Create a function named divisors/Divisors that takes an integer and returns an array with all of the integer's divisors(except for 1 and the number itself). If the number is prime return the string '(integer) is prime' (null in C#)

public static int[] Divisors(int n /* out int numfactors*/)
{
    List<int> divArray = new List<int>(); 
    int div;

    if (isPrime(n))
    {
        return divArray.ToArray();
    }
    else
    {

        for (div = 2; div < n / 2 + 1; div++)
        {
            if (n % div == 0)
            {
                divArray.Add(div);
            }

            return divArray.ToArray();
        }
    }
}

public static bool isPrime(int n)
{
    int d = 2;

    if (n == 1 && n % 2 == 0 && n != 2) return false;

    while (d * d <= n)
    {
        if (n % d == 0) return false;

        d = d + 1;
    }

    return true;
}

What i'm doing wrong, how I could optimize this algorithm? If I test a prime number, my code return "nothing" this I think it's another problem. if the number is prime, and I'm trying to return null my program crashes with : "Object reference not set to an instance of an object"

Since your code does not produce correct results it can be optimized to always return same value thus continue current behavior. If that's not enough you need to scope down question to single proble with solid [mcve]. — Alexei Levenkov, Nov 08 '17 at 22:43
The challenge doesn't want you to calculate whether or not the number is prime - it wants you to calculate and return all of the positive divisors (except 1 and the number itself). Only when there are none (and thus it is prime) return "n is prime". — PeteGO, Nov 08 '17 at 22:46
That return inside the for...loop seems to be totally wrong. I am curious to see the _main_ of this snippet — Steve, Nov 08 '17 at 22:49
@Steve: static void Main(string[] args) { //var watch = System.Diagnostics.Stopwatch.StartNew(); int[] divizori = Divisors(24); for (int i = 0; i < divizori.Length; i++) { Console.Write(divizori[i] + " "); } //watch.Stop(); Console.WriteLine(); //Console.WriteLine(watch.ElapsedMilliseconds); } My output is : 2 3 4 6 8 12 — Andrei Gandi, Nov 08 '17 at 22:57
@AlexeiLevenkov Did this get moved? I thought we were on SO? — NetMage, Nov 08 '17 at 23:21

score 0 · Accepted Answer · answered Nov 08 '17 at 23:27

In the comments on your question, others have noted that your program has correctness issues that you need to address before you worry about performance, and that's absolutely true. However, since you asked about performance, consider that the while loop in IsPrime and the for loop in Divisors are both doing essentially the same thing: iterating through a set of potential factors and determining whether they evenly divide n. So:

Why are you doing this twice? Since you need to produce a complete list of factors in the case of a composite n, why not just produce the whole list upfront and then infer whether n is prime from the size of the list?
More importantly, why is the for loop in Divisors using n/2+1 as its upper bound when your while loop in IsPrime correctly observes that you only really need to go up to √n? Yes, a composite n will have factors that are greater than √n, but you don't need to iterate above that point to find them, because for any k that evenly divides n, you know that n/k also evenly divides n.

Thank you @joefarrel. Now I realize what I'm wrong with and I did not understand the exact requirement. — Andrei Gandi, Nov 08 '17 at 23:37

How to optimize this c# code?

1 Answers1