# VBuzz all fizzed up

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

There's something about 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
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 , which is what I wanted to have here - idea being to keep the -specifics in a -specialized class.

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

-

Things I like:

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

End Sub

-

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

-
I felt the same way until I read this answer to a different fizzbuzz question. codereview.stackexchange.com/a/33724/41243 –  RubberDuck Jul 13 '14 at 13:19
As long as the results are proper having the code be more efficient helps in extreme cases. For instance following this pattern, in c++, gave me a top ranking score on CodeEval. –  tinstaafl Jul 13 '14 at 21:02
I also read codereview.stackexchange.com/questions/33717/… and still feel the same way I did when I commented on it. Concatenation is the standard principle behind fizzbuzz. In real-life fizzbuzz you are much more likely to want to add new rules with new words than to want to make a special output for 15 that isn't just a concatenation of the 3-output and 5-output. –  David K Jul 23 '14 at 6:45