Display factorial with user-entered number

This needs some optimizing without any additional variables or loops. The difficult part of the task was no additional variables subroutines/functions or loops. If that seems (near) impossible then break those rules.

The output should look like:


5 X 4 X 3 X 2 X 1 = 120

Private Sub FactorialCalculation()

 Dim Number, Factorial As Integer

 Console.WriteLine("Please enter a number")
 Number = CInt(Console.ReadLine)
 Factorial = 1

 Console.WriteLine("Your number is " & Number)

 If Number > 0 Then
     Console.Write(Number & " X ")
     For Count As Integer = 1 To Number
         If (Number - Count) > 0 And (Number - Count) <> 1 Then
             Factorial *= Count
             Console.Write("{0} X ", Number - Count)
         ElseIf (Number - Count) = 0 Then
            Factorial *= Count
         ElseIf (Number - Count) = 1 Then
             Factorial *= Count
             Console.Write(Number - Count)
         End If

     Next
     Console.WriteLine(" = {0}", Factorial)
 Else
     Console.WriteLine("Please enter a positive number greater than 0")
 End If
End Sub

The difficult part of the task was no additional variables, subroutines/functions, or loops.

I'll keep that in the back of my head, but it's far more important to know what tools to use when, than being able to implement functionality x using language features y or z.

It's also much more important to know how to separate concerns and why - this "no additional subroutines/functions" so-called requirement is a very questionable idea, especially if that's something someone is tasking you to do.

In the real world, your client (or boss, or project manager, or whatever) tells you what the program needs to do: how it does it is at your [team's] discretion, and you[r team] will want to write code that's easy to maintain and extend, because that client never seems to stop adding new requirements - and if you do it wrong, one day a new requirement will come and shatter some fundamental assumption your code was making, and you need to rewrite a big part of it, and then doing so you introduce bugs that you will want to identify and fix quickly.

Writing a "god procedure" that literally does everything goes against every sane design principle, so I'm going to toss that non-functional bogus requirement away - because what you really want is to write maintainable, extensible code that's easy to read and easy to debug.

The "god procedure" also fails at abstraction, which is one of the four pillars of Object-Oriented Programming (OOP), and happens to be the paradigm VB.NET was designed for.

The functional requirements are:

• User needs to be able to input a number, which must be a positive integer.

• The program needs to compute the factorial of that number.

• The program needs to output the result and the calculations made to get there.

I'll take the specified output as a firm functional requirement, and add:

• For input 5, the program should output 5 X 4 X 3 X 2 X 1 = 120.

That's what the program needs to do - these are the main concerns. We'll see if additional variables or loops are needed as we see fit. Let's start.

Review

I like starting with the signature:

Private Sub FactorialCalculation()

Procedures do something; they're actions. By convention, procedure names start with a verb, and describe what's going on. Their parameters have meaningful names, like local variables; nouns that describe their purpose.

Naming is already hard, but a procedure that does too many things is even harder to name accurately: we know FactorialCalculation has something to do with calculating factorials, but what's going on here is impossible to tell just by reading the name of the procedure: even CalculateFactorial would be a misleading name, because it doesn't talk about user input. Looking at the code, the only name that comes to mind is PromptAndValidateUserInputAndCalculateFactorialAndOutputResult, which sounds ridiculous for a reason.

Dim Number, Factorial As Integer

If you're writing VBA as well as VB.NET, this is a dangerous way to declare your variables: in VBA Number would be an implicit Variant, and while it's great that VB.NET fixed that annoyance, it's better to declare variables on separate instructions, as close as possible to their first use - and ideally assigning them a value.

Console.WriteLine("Please enter a number")
Number = CInt(Console.ReadLine)

CInt is perhaps a VB-ish way to do this, but as a C# dev looking at it, you wonder how that conversion function is going to do with a non-numeric string input, for example - whereas more .NET-idiomatic conversion methods leave no ambiguity about what's going on:

Dim input As String = Console.ReadLine()
Dim value As Integer
Dim isInteger As Boolean = Integer.TryParse(input, value)

So, the output needs to be in descending order. Why is the loop ascending then?

For Count As Integer = 1 To Number

Seeing how almost every reference to Count is for computing Number - Count, I would have reversed the loop:

For Count As Integer = Number To 1 Step -1

Lastly, in order to know whether the output matches what's expected, we need to mentally iterate that loop and "build" the output - that's not ideal.

There's a better way.

Testability

Let's say you want to test whether your program fulfills all the requirements. You can run the program and see it with your own eyes - and that's a mandatory thing to do every once in a while. But testing your code like this, for every single edge case, and re-doing all the tests every time you make a change to the code, gets tedious. Professional code is testable, meaning that you can write code that executes your program and verifies that it works correctly: here there's only a handful of points to clear, but in a real code base there could be thousands of tests that run every time something changes in the code. These tests not only help identifying regression bugs, they also document the requirements and assumptions the code is making.


QA engineer walks into

StackExchange Code Review Q#147152, answer score: 3