Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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
            Console.ReadLine()
        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?

share|improve this question

2 Answers 2

up vote 5 down vote accepted

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

    Console.ReadLine()
End Sub
share|improve this answer

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
share|improve this answer
    
I felt the same way until I read this answer to a different fizzbuzz question. codereview.stackexchange.com/a/33724/41243 –  ckuhn203 Jul 13 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 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 at 6:45

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.