3
\$\begingroup\$

This came from this question.

Find all the positive divisors of an integer >= 2.

Can stop when i * i >= number.

Please review for speed and style.

public class IntWithDivisors
{
    public override int GetHashCode()
    {
        return Number;
    }
    public override bool Equals(object obj)
    {
        if(obj is IntWithDivisors)
        {
            return ((IntWithDivisors)obj).Number == this.Number;
        }
        return false;
    }
    public override string ToString()
    {
        return $"number {Number}    Devisors " + string.Join(", ", Divisors);
    }

    public List<int> Divisors { get; } = new List<int>();
    public int Count { get { return Divisors.Count(); } }
    public int Number { get; }
    public IntWithDivisors(int number)
    {
        if (number < 2)
        {
            throw new ArgumentOutOfRangeException();
        }
        Number = number;
        Divisors = IntDivisors(number);
    }               
}
public static List<int> IntDivisors(int num)
{
    //Debug.WriteLine($"\nIntDevisors  {num}");
    List<int> intDivisors = new List<int>();
    intDivisors.Add(1);
    intDivisors.Add(num);
    int i;
    int incr;
    if(num / 2 * 2 == num)
    {
        i = 2;
        incr = 1;
    }
    else
    {
        i = 3;
        incr = 2;
    }
    for(; i*i < num; i += incr)
    {
        int numOveri = num / i;
        if (numOveri * i == num)
        {
            //Debug.WriteLine(i);
            intDivisors.Add(i);
            intDivisors.Add(numOveri);
        }
    }
    if(i*i == num)
    {
        intDivisors.Add(i);
    }
    intDivisors.Sort();
    return intDivisors;
}
\$\endgroup\$
3
\$\begingroup\$

Why is the implementation of the algorithm a static method outside of the class IntWithDivisors? Is it in class Program? It shouldn't be there.

It is unusual to do complex work in the constructor. A constructor should only perform initialization. See answer of Telastyn in how complex a constructor should be. There are different possibilities to solve this

E.g. lazy evaluation:

private List<int> _divisors;
public List<int> Divisors
{
    get {
        if (_divisors == null) {
            _divisors = IntDivisors(Number);
        }
        return _divisors;
    }
}

Another option is to simply call a method to get the result.

This leads us to the next question: what is the task of the class IntWithDivisors? Do we really need to store the input number together with the output? Do we really need to override Equals and GetHashCode?

I would rather opt for a minimalist but reusable and flexible approach in the LINQ style as extension method implemented as iterator.

I observed that the starting i is one bigger than incr. We can use this fact to simplify the initialization.

Testing if a number is even is usually done with the modulo operator which yields the remainder of the division num % 2 == 0.

numOveri is a strange name. I renamed it to quotient and i to divisor.

The divisors are tested in ascending order; however, the quotients accumulate in descending order. Therefore, we can return the divisors immediately with yield return and store the quotients in a list. We then need to reverse this list before we return its items.

public static class IntExtensions
{
    public static IEnumerable<int> SelectDivisors(this int num)
    {
        yield return 1;

        int incr = num % 2 == 0 ? 1 : 2;
        var largeDivisors = new List<int>();
        for (int divisor = incr + 1; divisor * divisor <= num; divisor += incr) {
            int quotient = num / divisor;
            if (quotient * divisor == num) {
                yield return divisor;
                if (quotient != divisor) {
                    largeDivisors.Add(quotient);
                }
            }
        }

        largeDivisors.Reverse();
        for (int k = 0; k < largeDivisors.Count; k++) {
            yield return largeDivisors[k];
        }

        yield return num;
    }
}

We can use this extension method like this (shown in a little test routine):

int[] numbers = new[] { 9, 12, 15, 16, 17, 27, 54 };
foreach (int num in numbers) {
    Console.Write($"Number {num} has divisors ");
    foreach (int n in num.SelectDivisors()) {
        Console.Write(n + " ");
    }
    Console.WriteLine();
}
\$\endgroup\$
  • \$\begingroup\$ Thanks. My though on public static List<int> IntDivisors is so that it could be called directly. You want divisors on a single number. If a class can define gethashcode and equals then I always implement. I like the better names. Quotient is not really a good name. It is the BigDivisor. So maybe DivisorSmall and DivisorBig. In my testing n / i * i was faster then modus. \$\endgroup\$ – paparazzo Jun 2 '18 at 16:58
  • \$\begingroup\$ I like IntExtensions and the reverse to avoid a sort. \$\endgroup\$ – paparazzo Jun 2 '18 at 17:04
  • \$\begingroup\$ Well, the quotient can be a divisor or not, depending on whether there is a remainder. But these namings are always a matter of taste. As long as they are understandable, they're all okay. \$\endgroup\$ – Olivier Jacot-Descombes Jun 2 '18 at 17:16
  • \$\begingroup\$ Not arguing. But divisor can also be a divisor or not, depending on whether there is a remainder. Great feedback. \$\endgroup\$ – paparazzo Jun 2 '18 at 17:44
-3
\$\begingroup\$

What I ended up with

public static class IntExtensions
{
    public static IEnumerable<int> SelectDivisors(this int num)
    {
        yield return 1;

        int incr = (num / 2 * 2 == num) ? 1 : 2;
        var largeDivisors = new List<int>();
        for (int smallDivisor = incr + 1; smallDivisor * smallDivisor <= num; smallDivisor += incr)
        {
            int largeDivisor = num / smallDivisor;
            if (largeDivisor * smallDivisor == num)
            {
                yield return smallDivisor;
                if (largeDivisor != smallDivisor)
                {
                    largeDivisors.Add(largeDivisor);
                }
            }
        }

        for (int k = largeDivisors.Count - 1; k >= 0; k--)
        {
            yield return largeDivisors[k];
        }

        yield return num;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Why would posting what I ended up with get down votes? \$\endgroup\$ – paparazzo Jun 3 '18 at 16:47
  • 2
    \$\begingroup\$ Self answers should be answers as well. This is just a code dump without a review of the code. If you can't add anything new you should refuse self-answers and better ask a follow-up question. \$\endgroup\$ – Heslacher Jun 4 '18 at 4:31
  • 1
    \$\begingroup\$ @paparazzo "If you want to show everyone how you improved your code, but don't want to ask another question, then post an answer to your own question"... But remember: "Your answer must meet the standards of a Code Review answer, just like any other answer. Describe what you changed, and why." - from the section _I improved my code based on the reviews. What next? on Help Center > Asking > What should I do when someone answers my question? \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jun 5 '18 at 15:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.