FizzBuzz Kickoff

Question

My first FizzBuzz question (well, I asked, then deleted, one previously), and my very first VB.NET question.

Module Module1
    Function FizzBuzzify(ByVal Value As Integer) As String
        If Value Mod 5 = 0 And Value Mod 3 = 0 Then
            Return "FizzBuzz"
        End If

        If Value Mod 3 = 0 Then
            Return "Fizz"
        End If

        If Value Mod 5 = 0 Then
            Return "Buzz"
        End If

        Return Value
    End Function

    Sub FizzBuzz(ByVal MinValue As Integer, ByVal MaxValue As Integer)
        For index = MinValue To MaxValue
            Console.WriteLine(FizzBuzzify(index))
        Next
    End Sub

    Sub Main()
        FizzBuzz(1, 100)
    End Sub
End Module

Please tell me the good, bad, and awful so I do, don't, and don't do the same in my next venture into VB land.

Answer 1

The Good

• This is, in my opinion, one of the easier to maintain and understand algorithms.
• You separated the concerns of calculating and writing. This is often overlooked for this particular challenge.
• The FizzBuzz routine is flexible and works well beyond the hardcoded "1 to 100" range stated in the problem.

The Bad

• Just because the IDE likes to fight you doesn't mean good casing shouldn't be applied whenever possible. Value is an argument and should be camelCased.

• Get in the habit of surrounding boolean expressions in parenthesis. VB doesn't force you to like C# does, but it helps to clarify when = really means ==. For example:

  If (value Mod 3 = 0) Then
  

• You're not using Option Strict. It doesn't matter in this code, but get into the habit. There's nothing worse than an implicit type cast ruining your day.

The Awful

I'm chucking this under awful, but it's really not. It's simply the worst offense.

You're using magic numbers for the Divisors. Minimally, use constants. For maximum flexibility, pass them as arguments and calculate the GCD for the first test case. You could even set them to default values using Optional parameters as not to break existing client code.

---

Overall, move on. You're way past FizzBuzz as a dev.

Answer 2

The Good

I don't see single-letter identifiers, you're passing parameters ByVal explicitly, and you've extracted the logic into small, specialized procedures.

The Bad

Procedures are implicitly Public here; while consistent with the vb6 ancestor, it's confusing because the implicit access modifier for module/class members in other .net languages, is private.

Hence, you should always use an explicit access modifier for all members in VB; the only method that I would expect to be Public in that module, is Sub Main.

The Awful

Nothing awful here, really. Well done!