4
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ the classic way to do this is via recursion, that would require less variables (no need for Count) \$\endgroup\$ – pm100 Nov 16 '16 at 1:48
  • \$\begingroup\$ and one less loop too \$\endgroup\$ – pm100 Nov 16 '16 at 1:50
  • \$\begingroup\$ also your code cannot cannot calculate factorial > 12 it will overflow integer type. If you change to unsigned long (if vb supports it) you can get up to 20. Either way you should warn on input - just like you warn for -3 \$\endgroup\$ – pm100 Nov 16 '16 at 2:03
3
\$\begingroup\$

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 a bar. Orders a beer. Orders 0 beers. Orders 999999999 beers. Orders a lizard. Orders -1 beers. Orders a sfdeljknesv.

https://twitter.com/JimFraeErskine/status/527564769302433792

Before you even write a single line of code, you can look at the requirements and, knowing that a 32-bit integer input is maxed at 12 and a 64-bit integer is maxed at 20, start thinking of what you'll need to test:

  • Whether the user input is deemed valid with a negative integer
  • Whether the user input is deemed valid with sfdeljknesv for input
  • Whether the user input is deemed valid with a positive integer under 12
  • Whether the user input is deemed valid with a positive integer under 20
  • Whether the user input is deemed valid with a positive integer above 20
  • Whether the program logic correctly computes the factorial of 0
  • Whether the program logic correctly computes the factorial of a positive integer under 12
  • Whether the output format matches the specifications

That's 8 tests we know we can write right away. There's a development methodology called Test-Driven Development that works exactly like this: you actually write the tests, then you write the code that makes these tests pass. One of the benefits of that methodology is that it's impossible to write untestable code, since you're only ever implementing just enough code to make a test pass.

Your implementation will throw unhandled exceptions with several of these tests, but the problem is that it's impossible to test a single procedure that does everything.

Separating Concerns

If we wanted to merely be able to write these tests, we would need separate procedures for:

  • Validating user input
  • Computing the factorial of a given integer
  • Generating the output

One implementation that comes to my mind would leverage an iterator to Yield a counter, and an <Out()> ByRef parameter to output the current result. Something like this:

Public Iterator Function CalculateFactorial(ByVal value As Integer, <Out()> ByRef result As Integer) As IEnumerable(Of Integer)
    If result < 0 OrElse result > 12 Then Throw New ArgumentException()

    result = 1
    For i As Integer = value To 1 Step -1
        result *= i
        Yield i
    Next

End Function

The calling code could look like this:

Dim input As Integer = 12
Dim result As Integer
Dim values As IEnumerable(Of Integer) = CalculateFactorial(input, result)

Notice how decoupling the user input from the factorial-computing logic now allows us to test the logic with any given valid or invalid Integer input. Also notice that the logic is separated from the formatted output we're going to end up with.

If we wrote unit tests for it, we could already test all these:

  • Whether and how the program logic handles invalid input (throws an ArgumentException)
  • Whether the program logic correctly computes the factorial of 0 (returns 1 as expected)
  • Whether the program logic correctly computes the factorial of a positive integer under 12 (does it?)

To test the input validation logic, we would have to implement a separate function - one that takes a String input and returns a Boolean, for example:

Public Function IsValidInput(ByVal input As String, <Out()> ByRef result As Integer) As Boolean
    If String.IsNullOrEmpty(input) OrElse Not IsNumeric(input) Then Return False

    Dim value As Integer
    Dim isInteger As Boolean = Integer.TryParse(input, value)
    If isInteger AndAlso value > 0 AndAlso value < 12 Then
        result = value
        Return True
    End If
End Function

With that function we can test all these:

  • Whether the user input is deemed valid with a negative integer
  • Whether the user input is deemed valid with sfdeljknesv for input
  • Whether the user input is deemed valid with a positive integer under 12
  • Whether the user input is deemed valid with a positive integer under 20
  • Whether the user input is deemed valid with a positive integer above 20

And now the calling code looks like this:

Dim input As Integer
If IsValidInput(Console.ReadLine(), input) Then
    Dim result As Integer
    Dim values As IEnumerable(Of Integer) = CalculateFactorial(input, result)
Else
    Console.WriteLine("Invalid input.")
End If

All that's left to do, is to assemble the yielded values into a String, and output it. A dedicated function can do this:

Public Function GenerateOutput(ByVal IEnumerable(Of Integer) values, ByVal result As Integer) As String
    Return String.Format("{0} = {1}", String.Join(" X ", values), result)
End Function

And now this test can pass:

  • Whether the output format matches the specifications

Result

Putting it all together, the resulting code might look like this - notice how the higher abstraction level of the Main procedure makes it easy to instantly grasp what's going on, with as little effort as it takes to just read it; the output is sent to the console once, and all the lower-level code can be tested independently:

Public Sub Main()
    Dim input As Integer
    If IsValidInput(Console.ReadLine(), input) Then
        Dim result As Integer
        Dim values As IEnumerable(Of Integer) = CalculateFactorial(input, result)
        Console.WriteLine(GenerateOutput(values, result))
    Else
        Console.WriteLine("Invalid input.")
    End If
End Sub

Public Function IsValidInput(ByVal input As String, <Out()> ByRef result As Integer) As Boolean
    If String.IsNullOrEmpty(input) OrElse Not IsNumeric(input) Then Return False

    Dim value As Integer
    Dim isInteger As Boolean = Integer.TryParse(input, value)
    If isInteger AndAlso value > 0 AndAlso value < 12 Then
        result = value
        Return True
    End If
End Function

Public Iterator Function CalculateFactorial(ByVal value As Integer, <Out()> ByRef result As Integer) As IEnumerable(Of Integer)
    If result < 0 OrElse result > 12 Then Throw New ArgumentException()

    result = 1
    For i As Integer = value To 1 Step -1
        result *= i
        Yield i
    Next

End Function

Public Function GenerateOutput(ByVal IEnumerable(Of Integer) values, ByVal result As Integer) As String
    Return String.Format("{0} = {1}", String.Join(" X ", values), result)
End Function

Is this the best way to implement it? Probably not. But it's testable, so go ahead and implement these tests, and then refactor the code to get rid of the <Out()> parameter in CalculateFactorial - if you break anything in the process, the tests will tell you.

\$\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.