# Calculating the Collatz conjecture of a number

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 collatzConjectureNumber = 0;
while (true)
{
break;
else
{
}
}
var collatzNumberCollection = new List<int>();

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

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

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

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


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.

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

# 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);
}
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");
}


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

• Very nice insight, thanks for the kind response to what is a spaghetti code, did this in a hurry – Oscar Guillamon Nov 17 '16 at 12:52

You can simplify this extract:

Console.WriteLine("Enter an integer number into the program to calculate its Collatz Conjeture sequence:");
var collatzConjetureNumber = 0;
while (true)
{
break;
else
{

int collatzConjetureNumber = 0;