HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Calculating the Collatz conjecture of a number

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
numbertheconjecturecalculatingcollatz

Problem

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();

    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 o

Solution

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 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 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:");

Code Snippets

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

        yield return value;
    }
}
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();
}
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;
}
void Main()
{    
    var number = ReadNumber();
    var collatzConjecture = CollatzConjecture(number);
    PrintCollatzConjecture(number, collatzConjecture);
}
Console.WriteLine($"The entered number is {result}, calculating Collatz's sequence:");

Context

StackExchange Code Review Q#147288, answer score: 4

Revisions (0)

No revisions yet.