VBuzz all fizzed up

Question

I don't normally write vb.net code, and I would rather maintain a monstruous vb6 project than a simple vb.net one.

There's something about fizzbuzz going on this weekend, that pushed me to write this:

Namespace CR.Sandbox.VB.FizzBuzz

    Module Sandbox

        Public Sub Main()
            For index = 1 To 100
                Console.WriteLine(FizzBuzzer.Convert(index))
            Next
            Console.ReadLine()
        End Sub

    End Module

    Module FizzBuzzer

        Public Function Convert(ByVal value As Integer) As String

            If value Mod 15 = 0 Then
                Return "FizzBuzz"
            ElseIf value Mod 3 = 0 Then
                Return "Fizz"
            ElseIf value Mod 5 = 0 Then
                Return "Buzz"
            Else
                Return value.ToString()
            End If

        End Function

    End Module

End Namespace

I put Convert in its own module because my understanding is that a module in VB is analoguous to a static class in c#, which is what I wanted to have here - idea being to keep the fizzbuzz-specifics in a fizzbuzz-specialized class.

Is there something I should know about vb.net that would make this code better?

Answer 1

Things I like:

• You defined the requirements correctly.
• Module was probably the right call.
• You used index instead of i for your loop. That's Awesome-sauce.

Ok, so what could be done better?

3,5, and 15 are all magic numbers, so it's a good time to introduce some constants. However one of them is not like the others. 3 and 5 are conditions. 15 is the lowest common multiple of those conditions.

So, we have three choices:

1. Define 3 constants.

   Const fizzDivisor As Integer = 3
   Const buzzDivisor As Integer = 5
   Const fizzbuzzDivisor As Integer = 15
   

   Note that this requires that any future developer understand that 15 is the lowest common multiple of 3 and 5. Also, to update either condition, we have to update their lcm by hand. I don't know about you, but I'm lazy and forgetful. I only want to make changes in one place.

2. Use two constants and calculate the lowest common multiple.

   This sounds perfect. Until you realize that in order to be efficient, we would want to calculate it once; outside of the loop. That would bind together Main and FizzBuzzer in a way I wouldn't be comfortable with. In fact, they become so tightly bound that there would be no need for FizzBuzzer at all. You would just implement the whole program right in Main.

3. Accept that the "ugly" way to write it may be the best way.

   Public Function Convert(ByVal value As Integer) As String
       Const fizzDivisor As Integer = 3
       Const buzzDivisor As Integer = 5
   
       If (value Mod fizzDivisor = 0) And (value Mod buzzDivisor = 0) Then
           Return "FizzBuzz"
       ElseIf value Mod fizzDivisor = 0 Then
           Return "Fizz"
       ElseIf value Mod buzzDivisor = 0 Then
           Return "Buzz"
       Else
           Return value.ToString()
       End If
   
   End Function
   

   This way, there are no more magic numbers and the cost (both CPU and real life) of computing a lowest common multiple have been wiped way completely.

What the heck, let's go one step further and run all kinds of different FizzBuzz programs. Forget the constants; let's use parameters.

Public Function Convert(ByVal value As Integer, Optional ByVal fizzDivisor As Integer = 3, Optional ByVal buzzDivisor As Integer = 5) As String
    If (value Mod fizzDivisor = 0) And (value Mod buzzDivisor = 0) Then
        Return "FizzBuzz"
    ElseIf value Mod fizzDivisor = 0 Then
        Return "Fizz"
    ElseIf value Mod buzzDivisor = 0 Then
        Return "Buzz"
    Else
        Return value.ToString()
    End If
End Function

We're using Optional parameters with default values, so your existing implementation won't break. Anyone familiar with FizzBuzzer still gets the expected results just by passing a value to it, but now it's easy to implement many different variations of Fizzbuzz.

Public Sub Main()
    For index = 1 To 100
        Console.WriteLine(FizzBuzzer.Convert(index))
    Next

    For index = 1 To 100
        Console.WriteLine(FizzBuzzer.Convert(index),3,4))
    Next

    For index = 1 To 100
        Console.WriteLine(FizzBuzzer.Convert(index),4,7))
    Next

    Console.ReadLine()
End Sub

Answer 2

My take would be, rather than calling Mod 3 times, just call it twice and build a string accordingly. Also to pass the 2 numbers to compare against as parameters:

Public Function Convert(ByVal value As Integer, numfizz As Integer, numbuzz As Integer) As String
    Convert = ""
    If value Mod numfizz = 0 Then
        Convert += "Fizz"
    End If
    If value Mod numbuzz = 0 Then
        Convert += "Buzz"
    End If
    If Convert = "" Then
        Convert = value.ToString
    End If
End Function