Fizzbuzz of course

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 Fun

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

StackExchange Code Review Q#141479, answer score: 7