6
\$\begingroup\$

Description
My program checks the sum of 2 numbers to determine if it is divisible by a certain number (5 in this case). Divisible numbers are deemed usable (for another program I am running). Next it checks the individual integers to see if those are divisible/usable as well.

Problem
I want to use this same code for divisors 2 - 9 (each will have their own class). The problem is that this code seems a little repetitive and lengthy to do for each divisor.

I want to know if there is a more simplistic way of generating the same output. Or is my answer found in creating individual classes for each action (that way each change will be reflected across the divisors classes)?

Code

static void Main(string[] args)
{
    Challenge(7, 7);
}
static void Challenge(int num1, int num2)
{
    //finds the sum of the two variables
    int sum = num1 + num2;
    Console.WriteLine("The sum of {0} and {1} is...\n{2}", num1, num2, sum);

    #region SumCheck
    bool isDivisible;
    //checks if divisible by 5 and sets a value for 'isDivisible'
    if (sum % 5 == 0)
    {
        Console.WriteLine("\nThe sum is divisible by 5!");
        isDivisible = true;
    }
    else
    {
        Console.WriteLine("\nThe sum is not divisible by 5!");
        isDivisible = false;
    }

    //depending on value of 'isDivisible', returns certain functions
    if (isDivisible)
    {
        Console.WriteLine("This value is usable.");
        Console.WriteLine("\n\nThe remaining usable values are: ");

        for (int newVal = 0; newVal <= 55; newVal++)
        {
            if ((newVal % 5 == 0) && (newVal != sum))
            {
                Console.WriteLine(newVal);
            }
        }
    }
    else
    {
        Console.WriteLine("This value is not usable.");
        Console.WriteLine("\n\nThese values are considered usable: ");

        for (int newVal = 0; newVal <= 55; newVal++)
        {
            if (newVal % 5 == 0)
            {
                Console.WriteLine(newVal);
            }
        }
    }
    #endregion

    #region NumCheck
    bool isNumDivisible;
    //checks if divisible by 5 and sets a value for 'isNumDivisible'
    if ((num1 % 5 == 0) && (num2 % 5 == 0))
    {
        Console.WriteLine("\n\n\n{0} and {1} are both divisible by 5!", num1, num2);
        isNumDivisible = true;
    }
    else if ((num1 % 5 == 0) && (num2 % 5 != 0))
    {
        Console.WriteLine("\n\n\nOnly {0} is divisible by 5!", num1);
        isNumDivisible = true;
    }
    else if ((num1 % 5 != 0) && (num2 % 5 == 0))
    {
        Console.WriteLine("\n\n\nOnly {0} is divisible by 5!", num2);
        isNumDivisible = true;
    }
    else
    {
        Console.WriteLine("\n\n\n{0} and {1} are both not divisible by 5!", num1, num2);
        isNumDivisible = false;
    }

    //depending on value of 'isNumDivisible', returns certain functions
    if (isNumDivisible)
    {
        //if both are true
        if ((num1 % 5 == 0) && (num2 % 5 == 0))
        {
            Console.WriteLine("Both values are usable.");
            Console.WriteLine("\n\nThe remaining usable values are: ");

            for (int newVal = 0; newVal <= 55; newVal++)
            {
                if ((newVal % 5 == 0) && (newVal != num1) && (newVal != num2))
                {
                    Console.WriteLine(newVal);
                }
            }
        }
        //if 'num1' is true and 'num2' is false
        else if ((num1 % 5 == 0) && (num2 % 5 != 0))
        {
            Console.WriteLine("Only {0} is usable.", num1);
            Console.WriteLine("\n\nThe remaining usable values are: ");

            for (int newVal = 0; newVal <= 55; newVal++)
            {
                if ((newVal % 5 == 0) && (newVal != num1))
                {
                    Console.WriteLine(newVal);
                }
            }
        }
        //if 'num2' is true and 'num1' is false
        else
        {
            Console.WriteLine("Only {0} is usable.", num2);
            Console.WriteLine("\n\nThe remaining usable values are: ");

            for (int newVal = 0; newVal <= 55; newVal++)
            {
                if ((newVal % 5 == 0) && (newVal != num2))
                {
                    Console.WriteLine(newVal);
                }
            }
        }
    }
    //if both are false
    else
    {
        Console.WriteLine("Both values are not usable.", num2);
        Console.WriteLine("\n\nThese values are considered usable: ");

        for (int newVal = 0; newVal <= 55; newVal++)
        {
            if (newVal % 5 == 0)
            {
                Console.WriteLine(newVal);
            }
        }
    }
    #endregion

    Console.ReadLine();
}
\$\endgroup\$
  • 1
    \$\begingroup\$ That code looks way overcomplicated. \$\endgroup\$ – πάντα ῥεῖ Dec 7 '15 at 21:20
  • \$\begingroup\$ lol, I figured... that's why I posted it. \$\endgroup\$ – ITSUUUUUH Dec 7 '15 at 21:51
4
\$\begingroup\$

Instead of handling only 5, it would be good to generalize to all divisors 2-9.

Avoid magic numbers like 55, scattered in many places across the code. It would be better to centralize the logic, and parameterize it, without hard-coding a specific number.

The boolean logic is also a bit dirty and hard to read.

Consider this alternative implementation that addresses the above points:

    static void Main(string[] args)
    {
        for (int i = 2; i < 10; i++)
        {
            Challenge(7, 7, i);
        }
        Console.ReadLine();
    }

    static void Challenge(int num1, int num2, int Divisor)
    {
        int sum = num1 + num2;
        bool SumDivisible = sum % Divisor == 0;
        bool num1Divisible = num1 % Divisor == 0;
        bool num2Divisible = num2 % Divisor == 0;

        int highNum = 55;
        List<int> NumbersDivisible = Enumerable.Range(1, highNum).Where(x => x % Divisor == 0).ToList();

        // Use the booleans to determine output.
        if (SumDivisible || num1Divisible || num2Divisible)
        {
            if (SumDivisible)
            {

                outputListExceptInt(NumbersDivisible, sum);
                //output
            }
            if (num1Divisible)
            {
                outputListExceptInt(NumbersDivisible, num1);
                //output
            }

            if (num2Divisible)
            {
                outputListExceptInt(NumbersDivisible, num2);
                //output
            }
        }
        else
        {
            outputListExceptInt(NumbersDivisible);
        }
    }

    public static void outputListExceptInt(List<int> NumbersDivisibleByDivisor, int except = 0)
    {
        var Numbers = except > 0 ? NumbersDivisibleByDivisor.Where(x => x != except) : NumbersDivisibleByDivisor;
        foreach (int num in Numbers)
        {
            Console.WriteLine(num);
        }
    }
\$\endgroup\$
  • \$\begingroup\$ Was waiting for your edit. This is perfect for what I am trying to do. I'll upvote your post as soon as my privilege allows me to. Thanks! \$\endgroup\$ – ITSUUUUUH Dec 7 '15 at 22:28
  • 1
    \$\begingroup\$ No problem. Glad to be of help! :) \$\endgroup\$ – wentimo Dec 7 '15 at 22:47
  • 1
    \$\begingroup\$ Hi @wentimo, I rewrote your text, to make it more about the code under review that "what you've done". I think it's a lot easier to understand. If you don't like it, feel free to roll back my edit, but I really think it's a much better Code Review answer this way, and I suggest to follow this writing style in your future posts. \$\endgroup\$ – janos Dec 30 '15 at 21:36
  • \$\begingroup\$ @janos Thanks for the edit! Definitely helps clarify what the main problems in my code were. \$\endgroup\$ – ITSUUUUUH Dec 31 '15 at 16:59
4
\$\begingroup\$

Why would you make each one it's own class? All you need is a property:

public int Divisor { get; }

Then, wherever you have "5" in you code, use Divisor. This means you could make a constructor:

public ClassName(int divisor)
{
    Divisor = divisor;
}

If you don't have access to C#6.0 (which the above requires) you can use:

public int _divisor;
public int Divisor { get { return _divisor; } }

public ClassName(int divisor)
{
    _divisor = divisor;
}

Then, define your challenge method:

public void Challenge(int num1, int num2)
{
    // code here
}

If you also want to auto-generate that 55 in:

for (int newVal = 0; newVal <= 55; newVal++)

You can do:

for (int newVal = 0; newVal <= _divisor * 11; newVal++)

Which will give you the same format of equality. (E.g.: <= 22 for 2, etc.)

You should also consider adding an exception in the constructor we defined above if divisor is outside the range you would like.

\$\endgroup\$
  • \$\begingroup\$ Other than the need of a property, were there any prevalent mistakes in logic for my current program that could be simplified? Or is it too complicated to even begin? \$\endgroup\$ – ITSUUUUUH Dec 7 '15 at 21:38
  • \$\begingroup\$ Accepted @wentimo 's post as it solved my problem. Your post helped in my logic, so I wish I could accept both... I appreciate all of your help though! (will upvote as soon as privilege allows me to) \$\endgroup\$ – ITSUUUUUH Dec 7 '15 at 22:31

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.