9
\$\begingroup\$

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
\$\endgroup\$
2
  • 4
    \$\begingroup\$ This would be more "useful" as a function that returns a string, leaving the responsibility of displaying it elsewhere. \$\endgroup\$ – RubberDuck Dec 3 '15 at 11:23
  • 1
    \$\begingroup\$ I agree with @RubberDuck here, it would make a lot of sense as a function that returns the solution to its cell \$\endgroup\$ – Raystafarian Dec 3 '15 at 12:08
7
\$\begingroup\$

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

1 Short-circuiting in VBA is ugly

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Thinking so much about FizzBuzz seems a little crazy. :-D \$\endgroup\$ – Daniel Dec 3 '15 at 15:56
9
\$\begingroup\$
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.

\$\endgroup\$
5
\$\begingroup\$

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

\$\endgroup\$
2
  • \$\begingroup\$ It's just personal preference really. I like verbalising what my check is meant to represent. \$\endgroup\$ – Kaz Dec 3 '15 at 19:36
  • \$\begingroup\$ It's useful for more complicated elements. There, the alternative is rather obvious. \$\endgroup\$ – gazzz0x2z Dec 3 '15 at 19:39

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.