Problem
FizzBuzz in VBA. Takes start and end numbers as arguments. Will handle any pair of integers to +- 2 Billion or so, determine whether the sequence is increasing or decreasing and output numbers to the immediate window.
Public Sub PrintFizzBuzz(ByVal startNum As Long, ByVal endNum As Long)
Dim currentNum As Long
Dim stepValue As Long
Dim outputString As String
Dim isFizzBuzz As Boolean
If startNum <= endNum Then stepValue = 1 Else stepValue = -1
For currentNum = startNum To endNum Step stepValue
outputString = ""
isFizzBuzz = False
If currentNum Mod 3 = 0 Then
outputString = outputString & "Fizz"
isFizzBuzz = True
End If
If currentNum Mod 5 = 0 Then
outputString = outputString & "Buzz"
isFizzBuzz = True
End If
If Not isFizzBuzz Then outputString = CStr(currentNum)
Debug.Print outputString
Next currentNum
End Sub
Solution
As mentioned by @RubberDuck in the comments, I’d suggest moving the logic within your for loop to a function:
...
For currentNum = startNum To endNum Step stepValue
Debug.Print GetFizzBuzz(currentNum)
Next currentNum
...
Private Function GetFizzBuzz(ByVal num As Long) As String
outputString = ""
isFizzBuzz = False
If num Mod 3 = 0 Then
outputString = outputString & "Fizz"
isFizzBuzz = True
End If
If num Mod 5 = 0 Then
outputString = outputString & "Buzz"
isFizzBuzz = True
End If
If Not isFizzBuzz Then outputString = CStr(num)
GetFizzBuzz = outputString
End Function
You could make that function public and call it directly for any number.
Like gazzz0x2z, using a boolean like the way you have kind of bothers me. I would prefer a function which would indicate the result and exit early:
Private Function IsFizzBuzz(ByVal num As Long) As Boolean
Select Case True 'Use Select Case for Short Circuiting :-( 1
Case num Mod 3 = 0, num Mod 5 = 0
IsFizzBuzz = True
Exit Function
Case Else
IsFizzBuzz = False
End Select
End Function
You could then add a check with exit to the beginning of the GetFizzBuzz function:
If IsFizzBuzz(num) Then
GetFizzBuzz = CStr(num)
Exit Function
End If
If startNum <= endNum Then stepValue = 1 Else stepValue = -1
For currentNum = startNum To endNum Step stepValue
You’re evil. Eeeeeee-vil. Properly indented:
If startNum <= endNum Then
stepValue = 1
Else
stepValue = -1
End If
For currentNum = startNum To endNum Step stepValue
Indenting that For
block under the line that starts with If
causes a shortcut in my brain: I skip the rest of the If
line and read the For
as part of the If
block.
Actually, since both alternatives are constant expressions1, you could use IIf
here:
stepValue = IIf(startNum <= endNum, 1, -1)
For currentNum = startNum To endNum Step stepValue
outputString = ""
This empty string literal takes up two bytes, when you could have used a null string pointer and used none:
outputString = vbNullString
1 The IIf
function evaluates both true and false results, so you don’t want side-effects.
Just nitpicking, but the isFizzBuzz Boolean sees not useful to me.
If Not isFizzBuzz Then outputString = CStr(currentNum)
Can be replaced by
If outputString = "" Then outputString = CStr(currentNum)
I don’t think it has a negative impact upon readability. You ditch 3 lines, and you are making a genuine functional test. Afterall, you’re writing the number because you were unable to write the letters.
(plus everything Mat’s Mug said, so if you go the vbNullString path, adapt my code).