3
\$\begingroup\$

I've made a Collatz Conjecture sequence calculator and I would like some feedback on what I can do better in regards to the code I've implemented, i.e. bad practices you see, improvements that can be made to make it faster, errors that might arise from this code that I am not managing, etc.

public static void CollatzConjecture()
{
    Console.WriteLine("Enter an integer number into the program to calculate its Collatz Conjecture sequence:");
    var consoleReading = string.Empty;
    var collatzConjectureNumber = 0;
    while (true)
    {
        consoleReading = Console.ReadLine();
        if (int.TryParse(consoleReading, out collatzConjectureNumber))
            break;
        else
        {
            Console.WriteLine("Please insert an integer number:");
        }
    }
    var collatzNumberCollection = new List<int>();

    Console.WriteLine("The entered number is {0}, calculating Collatz's sequence:", collatzConjectureNumber);

    collatzNumberCollection.Add(collatzConjectureNumber);

    int collatzStepCounter = 0;
    while (collatzConjectureNumber > 1)
    {
        collatzConjectureNumber = (collatzConjectureNumber%2).Equals(0)
            ? collatzConjectureNumber / 2
            : collatzConjectureNumber * 3 + 1;

        collatzNumberCollection.Add(collatzConjectureNumber);

        Console.WriteLine("Element {0} of the {1}'s collatz sequence is: {2}", ++collatzStepCounter, collatzNumberCollection[0], collatzConjectureNumber);
        Thread.Sleep(100);
    }

    Console.WriteLine("The Collatz Conjecture sequence for the number {0} contains {1} numbers", collatzNumberCollection[0], collatzNumberCollection.Count);

    Console.WriteLine("The Collatz Conjecture sequence has been calculated, press enter key to exit");
    Console.ReadLine();
}

The code works as intended but, as said, I would like feedback on what I can improve in this code. That explains why I post this question even though there exist duplicates of this one, I just want feedback on my code, that's all.

Thanks in advance for your responses.

\$\endgroup\$
  • \$\begingroup\$ It's bugging me that nobody on this page has yet corrected the spelling of "conjecture" (note the "c" in the middle). \$\endgroup\$ – IMSoP Nov 17 '16 at 13:27
4
\$\begingroup\$

Single Responsibility Principle

The CollatzConjecture method is the so called god-method. What does this mean? It does everything. But what exactly? I has three responsibilities:

  • read the number from the console
  • calculate the Collatz Conjecture
  • print the results

Each of those features should be encapsulated in their own method. But why? So that you can better test it. Currently you cannot write a unit test to validate whether the calculation is correct because it's bound to the console so you need to extract the math first:

public static IEnumerable<int> CollatzConjecture(int value)
{
    while (value > 1)
    {
        value = 
            (value % 2) == 0
            ? value / 2
            : value * 3 + 1;

        yield return value;
    }
}

Now you have a deferred method that does the math. Notice the yield return.


You could already use it but you want to see the output so you need another method that can print it. Here it is:

public static void PrintCollatzConjecture(int number, IEnumerable<int> collatzConjecture, int delayInMilliseconds = 100)
{
    var counter = 0;
    foreach (var value in collatzConjecture)
    {
        Console.WriteLine("Element {0} of the {1}'s collatz sequence is: {2}", ++counter, number, value);
        Thread.Sleep(delayInMilliseconds);
    }
    Console.WriteLine("The Collatz Conjecture sequence for the number {0} contains {1} numbers", number, counter);
    Console.WriteLine("The Collatz Conjecture sequence has been calculated, press enter key to exit");
    Console.ReadLine();
}

No magic here. Just an ordinary loop that prints each element. Additionally you can specify a different delay.


To make it interactive you need to create the third method that will read the number from the user:

public static int ReadNumber()
{
    Console.WriteLine("Enter an integer number into the program to calculate its Collatz Conjecture sequence:");

    var result = 0;
    var input = (string)null;

    var isValidNumber = true;
    do
    {
        if (!isValidNumber)
        {
            Console.WriteLine("Please insert an integer number:");
        }
        input = Console.ReadLine();
        isValidNumber = int.TryParse(input, out result);
    } while (!isValidNumber);

    Console.WriteLine("The entered number is {0}, calculating Collatz's sequence:", result);

    return result;
}

All three features are now encapsulated. You are free to modify and optimize any of them without breaking the other two. This is what we always strive for. One module/class/method does only one thing at a time.


If you assemble everything your Main will be nice and short:

void Main()
{    
    var number = ReadNumber();
    var collatzConjecture = CollatzConjecture(number);
    PrintCollatzConjecture(number, collatzConjecture);
}

C# 6

With the latest C# there is an easier way to build string. You can put the variables inside them. In order to do this you need to prefix it with the $ e.g.:

Console.WriteLine($"The entered number is {result}, calculating Collatz's sequence:");
\$\endgroup\$
  • \$\begingroup\$ Very nice insight, thanks for the kind response to what is a spaghetti code, did this in a hurry \$\endgroup\$ – Oscar Guillamon Nov 17 '16 at 12:52
1
\$\begingroup\$

You can simplify this extract:

Console.WriteLine("Enter an integer number into the program to calculate its Collatz Conjeture sequence:");
var consoleReading = string.Empty;
var collatzConjetureNumber = 0;
while (true)
{
    consoleReading = Console.ReadLine();
    if (int.TryParse(consoleReading, out collatzConjetureNumber))
        break;
    else
    {
        Console.WriteLine("Please insert an integer number:");
    }
}

Into this:

int collatzConjetureNumber = 0;
do
    Console.WriteLine("Enter an integer number into the program to calculate its Collatz Conjeture sequence:");
while (!int.TryParse(Console.ReadLine(), out collatzConjetureNumber));
\$\endgroup\$

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.