Fizzbuzz of course

Question

My second attempt with vb.net - I tried taking the advice from yesterday's question into consideration, but I did not try using LINQ.

You know the drill:

A programming challenge that prints the numbers from 1 to 100, where multiples of three print "Fizz," multiples of five print "Buzz," and multiples of both three and five print "FizzBuzz."

• Input is a list of integers 1-100 comma-separated in a txt file.
• Output is the list of fizzbuzz in the same format.

Code:

Option Explicit On
Option Strict On
Option Infer On
Option Compare Text
Imports System.Windows.Forms
Imports System.IO

Module FizzBuzz

    Sub Main()
        Dim arrayOfStrings As String() = GetInput("C:\temp\fizzbuzz.txt")
        Dim arrayOfIntegers = ConvertMe(arrayOfStrings)
        arrayOfStrings = FizzBuzzMe(arrayOfStrings, arrayOfIntegers)
        Dim concatenatedArray As String = WillYouBeMine(arrayOfStrings)
        WriteFizzBuzzFile(concatenatedArray)
    End Sub

    Private Function GetInput(ByVal path As String) As String()
        Dim returnText As String = File.ReadAllText(path)
        Dim returnArray As String() = returnText.Split(New String() {","}, StringSplitOptions.None)
        Return returnArray
    End Function

    Private Function ConvertMe(ByVal arrayOfStrings As String()) As Integer()
        Return Array.ConvertAll(arrayOfStrings, Function(str) Int32.Parse(str))
    End Function

    Private Function FizzBuzzMe(ByVal arrayOfStrings As String(), ByVal arrayOfIntegers As Integer()) As String()
        Dim i As Integer
        For i = 0 To arrayOfStrings.Length - 1
            If Not arrayOfIntegers(i) Mod 15 = 0 Then
                If arrayOfIntegers(i) Mod 3 = 0 Then
                    arrayOfStrings(i) = "fizz"
                ElseIf arrayOfIntegers(i) Mod 5 = 0 Then
                    arrayOfStrings(i) = "buzz"
                End If
            Else : arrayOfStrings(i) = "fizzbuzz"
            End If
        Next
        Return arrayOfStrings
    End Function

    Private Function WillYouBeMine(ByVal stringArray As String()) As String
        Return String.Join(",", stringArray)
    End Function

    Private Sub WriteFizzBuzzFile(ByVal concatenatedArray As String)
        Const NEW_FILE As String = "C:\Temp\fizzbuzzreturn.txt"
        Dim fileAuthor As New StreamWriter(NEW_FILE)
        If Not File.Exists(NEW_FILE) Then File.CreateText(NEW_FILE)
        fileAuthor.WriteLine(concatenatedArray)
        fileAuthor.Close()
    End Sub

End Module

Answer 1

You said beginner, which is good. I'm going to go into a few basic VB.NET constructs that should help you, and then cover a couple advanced ones in detail so that hopefully it's useful for future projects. :)

I know you said you didn't try to use LINQ, but I added an example of it (you can ignore it if you like) that shows the other form of LINQ that wasn't demonstrated in your last question. This may make it easier to understand.

---

So, first: there's no need to define arrayOfStrings in the FizzBuzzMe method as a parameter. It's not used as such, instead it's used as a pre-fill so that you already have an array of the right size, but there are better ways to do that:

Private Function FizzBuzzMe(ByVal arrayOfIntegers As Integer()) As String()
    Dim arrayOfStrings As New String(arrayOfIntegers.Length)
    Dim i As Integer
    For i = 0 To arrayOfStrings.Length - 1
        If Not arrayOfIntegers(i) Mod 15 = 0 Then
            If arrayOfIntegers(i) Mod 3 = 0 Then
                arrayOfStrings(i) = "fizz"
            ElseIf arrayOfIntegers(i) Mod 5 = 0 Then
                arrayOfStrings(i) = "buzz"
            End If
        Else : arrayOfStrings(i) = "fizzbuzz"
        End If
    Next
    Return arrayOfStrings
End Function

---

You should be Using the StreamWriter instead of opening and closing it manually. This is because StreamWriter implements IDisposable, which is almost always an indicator that it uses unmanaged resources, and should be properly freed. (In this case, .Close() will do this, but it's still a best practice to use the Using.)

You also do not need the File work in this method. It's automatically created when StreamWriter is opened.

Private Sub WriteFizzBuzzFile(ByVal concatenatedArray As String)
    Const NEW_FILE As String = "C:\Temp\fizzbuzzreturn.txt"
    Using fileAuthor As New StreamWriter(NEW_FILE)
        fileAuthor.WriteLine(concatenatedArray)
    End Using
End Sub

---

Generally, we recommend to use the alias type for all work instead of the strong type. The only time this occurs is in ConvertMe when you use Int32.Parse, usually we would use Integer.Parse.

Private Function ConvertMe(ByVal arrayOfStrings As String()) As Integer()
    Return Array.ConvertAll(arrayOfStrings, Function(str) Integer.Parse(str))
End Function

---

The WillYouBeMine function has two problems: meaningless name, and it doesn't encapsulate a useful feature. It's just a slightly quicker way of combining the strings, but it has no actual impact except using stack space. Just get rid of it and inline the String.Join call.

Sub Main()
    Dim arrayOfStrings As String() = GetInput("C:\temp\fizzbuzz.txt")
    Dim arrayOfIntegers = ConvertMe(arrayOfStrings)
    arrayOfStrings = FizzBuzzMe(arrayOfIntegers)
    Dim concatenatedArray As String = String.Join(",", arrayOfStrings)
    WriteFizzBuzzFile(concatenatedArray)
End Sub

---

Lastly, we're going to modify the algorithm in FizzBuzzMe just a bit. Instead of making three cases, we only need two.

Private Function FizzBuzzMe(ByVal arrayOfIntegers As Integer()) As String()
    Dim arrayOfStrings(arrayOfIntegers.Length) As String
    Dim i As Integer
    For i = 0 To arrayOfStrings.Length - 1
        arrayOfStrings(i) = ""

        If i Mod 3 = 0 Then
            arrayOfStrings(i) &= "fizz"
        End If

        If i Mod 5 = 0 Then
            arrayOfStrings(i) &= "buzz"
        End If
    Next
    Return arrayOfStrings
End Function

Because we initialize a new string array, it's guaranteed by .NET to be all null (Nothing) strings. This eliminates the need for the three If statements, and brings you down to just two conditions. Then you append fizz if it's a fizz line, and append buzz if it's a buzz line.

---

There is no need for GetInput to be as many lines as it is, we can shorten it into one. (ALL the variables are used in one location.)

Private Function GetInput(ByVal path As String) As String()
    Return File.ReadAllText(path).Split(New String() {","}, StringSplitOptions.None)
End Function

---

We don't generally declare an iterator variable (i) outside the loop.

For i As Integer = 0 To arrayOfStrings.Length - 1

---

Now, you don't take any input in this programme at all. You just run it procedurally and exit. While this is great for dummy programme's that demonstrate a system, you should consider adding support for user input through arguments or the CLI. I'm not going to detail all that here, but I will say that taking input through the CLI is extremely easy.

---

The next thing we can do is replace the result of FizzBuzzMe with an IEnumerable(Of String), and change it to an Iterator function. This allows us to be lazy in our implementation. We simply Yield whatever value we have at the moment.

Private Iterator Function FizzBuzzMe(ByVal arrayOfIntegers As Integer()) As IEnumerable(Of String)
    For i As Integer = 0 To arrayOfIntegers.Length - 1
        Dim line As String = ""

        If i Mod 3 = 0 Then
            line &= "fizz"
        End If

        If i Mod 5 = 0 Then
            line &= "buzz"
        End If

        Yield line
    Next
End Function

If you notice, we got rid of arrayOfStrings entirely. Instead, we're using an Iterator Function to only return lines as we process them. This means that if we're only looking for the first fizz, we aren't processing the entire collection, but instead only process until we find fizz. (I'm not going to write an example of this at this time of night, but it's pretty cool.)

The only other change we need, is add .ToArray() to the FizzBuzzMe(arrayOfIntegers) call:

Sub Main()
    Dim arrayOfStrings As String() = GetInput("C:\temp\fizzbuzz.txt")
    Dim arrayOfIntegers = ConvertMe(arrayOfStrings)
    arrayOfStrings = FizzBuzzMe(arrayOfIntegers).ToArray()
    Dim concatenatedArray As String = String.Join(",", arrayOfStrings)
    WriteFizzBuzzFile(concatenatedArray)
End Sub

---

Next, we'll do a little LINQ to make it pop just a bit:

Private Function FizzBuzzMe(ByVal arrayOfIntegers As Integer()) As IEnumerable(Of String)
    Return arrayOfIntegers.Select(Function(x)
                                      Dim value As String = ""
                                      If x Mod 3 = 0 Then value &= "fizz"
                                      If x Mod 5 = 0 Then value &= "buzz"
                                      Return value
                                  End Function)
End Function

Basically, the .Select method on an IEnumerable (which an array is) allows us to transform the elements from Integer to a String with some complex logic.

We can't (and don't need) to extend this to our Iterator version, as Select is already lazy.

---

Lastly, we can use the If(Expression, IfTrue, IfFalse) construct to simplify our FizzBuzzMe method:

Private Function FizzBuzzMe(ByVal arrayOfIntegers As Integer()) As IEnumerable(Of String)
    Return arrayOfIntegers.Select(Function(x)
                                      Return If(x Mod 3 = 0, "fizz", "") & If(x Mod 5 = 0, "buzz", "")
                                  End Function)
End Function

You could even one-line it pretty easily at this point:

Private Function FizzBuzzMe(ByVal arrayOfIntegers As Integer()) As IEnumerable(Of String)
    Return arrayOfIntegers.Select(Function(x) If(x Mod 3 = 0, "fizz", "") & If(x Mod 5 = 0, "buzz", ""))
End Function

---

Finally, the end of our code looks like:

Sub Main()
    Dim arrayOfStrings As String() = GetInput("C:\temp\fizzbuzz.txt")
    Dim arrayOfIntegers = ConvertMe(arrayOfStrings)
    arrayOfStrings = FizzBuzzMe(arrayOfIntegers).ToArray()
    Dim concatenatedArray As String = String.Join(",", arrayOfStrings)
    WriteFizzBuzzFile(concatenatedArray)
End Sub

Private Function GetInput(ByVal path As String) As String()
    Return File.ReadAllText(path).Split(New String() {","}, StringSplitOptions.None)
End Function

Private Function ConvertMe(ByVal arrayOfStrings As String()) As Integer()
    Return Array.ConvertAll(arrayOfStrings, Function(str) Integer.Parse(str))
End Function

Private Function FizzBuzzMe(ByVal arrayOfIntegers As Integer()) As IEnumerable(Of String)
    Return arrayOfIntegers.Select(Function(x)
                                      Return If(x Mod 3 = 0, "fizz", "") & If(x Mod 5 = 0, "buzz", "")
                                  End Function)
End Function

Private Sub WriteFizzBuzzFile(ByVal concatenatedArray As String)
    Const NEW_FILE As String = "C:\Temp\fizzbuzzreturn.txt"
    Using fileAuthor As New StreamWriter(NEW_FILE)
        fileAuthor.WriteLine(concatenatedArray)
    End Using
End Sub

We've reduced the complexity of certain portions of the code, and demonstrated a few complex code examples at the same time.

Input:

1,2,3,4,5,6,7,8,9,10,11,12,13,14,15

Output:

,,fizz,,buzz,fizz,,,fizz,buzz,,fizz,,,fizzbuzz

As you can see, the programme works just fine. I do hope you (and others) find this information helpful, and can learn more about VB.NET from it. :) Personally, I find VB.NET to be quite powerful, even though a lot of people act like it's not.

Answer 2

First, I would change the way you styled the if statements. Generally the less nesting you have the better.

I would go with the more simple:

If arrayOfIntegers(i) Mod 15 = 0 Then
    arrayOfStrings(i) = "fizzbuzz"
ElseIf arrayOfIntegers(i) Mod 3 = 0 Then
    arrayOfStrings(i) = "fizz"
ElseIf arrayOfIntegers(i) Mod 5 = 0 Then
    arrayOfStrings(i) = "buzz"
End If

Second, some of the names of your functions are a bit confusing or don't provide as much information as they should. For example, ConvertMe, Convert to what? I would change it to something like ConvertToIntArray. What does WillYouBeMine do and what is its purpose?

Third, the method that reads input doesn't have the file name hard coded into it, instead it takes it as a parameter. I would do the same thing for the output.

Fourth, not entirely necessary, but I would consider renaming the integer and string arrays to input and output. Their current name tells us what sort of information they hold, but not really what the purpose of that information is.

Answer 3

Just a couple of things to add to what @yitzih has already suggested:

Watch your imports

Try not to have imports in your code that you don't need. For example, in the code you've posted you've imported System.Windows.Forms even though you don't need it.

Split

Your split line seems overly verbose. The default behaviour is going to be SplitOptions.None, so you don't need to specify it. You can get away with just doing:

Dim returnArray As String() = returnText.Split({","c})

File Creation

You're performing a pointless check:

If Not File.Exists(NEW_FILE) Then File.CreateText(NEW_FILE)

This is done after Dim fileAuthor As New StreamWriter(NEW_FILE) which creates the file if it doesn't exist, so it's always going to be there for the File.Exists check. If you want to change the behaviour (from create new file to append, then you need to supply an extra parameter):

Dim fileAuthor As New StreamWriter(NEW_FILE, true)

Answer 4

Another way to go is to eliminate all the arrays and process each byte as it comes out of the stream. In many cases you'll find this faster than reading a string and converting it to a number:

Sub WriteResults(inFile As String, outFile As String)
    Using inStream As New System.IO.StreamReader(inFile)
        Using outStream As New System.IO.StreamWriter(outFile, False)
            Do Until inStream.EndOfStream
                Dim test As Integer = 0
                '44 is ASCII code for the comma
                Do Until inStream.Peek = 44 OrElse inStream.EndOfStream
                    test += inStream.Read - 48
                    Dim pek As Integer = inStream.Peek
                    If pek <> 44 AndAlso Not inStream.EndOfStream Then
                        test *= 10
                        test += inStream.Read - 48
                        pek = inStream.Peek
                    End If
                Loop
                inStream.Read()
                If test Mod 3 = 0 Then
                    outStream.Write("fizz")
                End If
                If test Mod 5 = 0 Then
                    outStream.Write("buzz")
                End If
                If Not inStream.EndOfStream Then
                    outStream.Write(",")
                End If
                test = 0
            Loop
        End Using
    End Using

End Sub