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:
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.
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.
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