7
\$\begingroup\$

I am trying to check a bunch of files that should have a leading | delimiter but sometimes don't. If the first line doesn't have the |, the rest of the file won't, if it does have it, then the rest of the file will.

I'm checking the first line which works and enters the update logic, which advances to the next row once I read the line in. So I added a writer.Write before I go into the Do While loop and it works, but it seems like there would be a better way of doing this.

    Dim fileList As FileInfo() = dirList.GetFiles

    For Each fiName In fileList
        sFullName = fiName.FullName
        sFileName = Path.GetFileNameWithoutExtension(fiName.ToString)
        sDir = fiName.DirectoryName
        Dim reader As New StreamReader(sFullName)
        Dim line As String = reader.ReadLine()

        If Not line.StartsWith("|") Then

            Dim sNewFullName As String = sDir + "\" + sFileName + "_Temp.txt"
            Dim writer As New StreamWriter(sNewFullName)
            Dim sLine As String

            sLine = "|" + line + System.Environment.NewLine
            writer.Write(sLine)

            Do While reader.Peek() <> -1
                sLine = "|" + reader.ReadLine() + System.Environment.NewLine
                writer.Write(sLine)
            Loop

            writer.Close()
            reader.Close()
            File.Delete(sFullName)
            File.Move(sNewFullName, sFullName)

        End If
        reader.Close()
    Next fiName
\$\endgroup\$
5
\$\begingroup\$
  • Building filenames should be done using the System.IO.Path.Combine() method. This ensures that you don't skip a "\" by accident and also enforces that no invalid path/file characters are used.
  • Strings shouldn't be added to other strings (if done more than once), better use System.Text.StringBuilder

As Mat's Mug already pointed out, your variable naming should be enhanced.

So

Dim sNewFullName As String = sDir + "\" + sFileName + "_Temp.txt"  

would become

Dim newFullName As String = System.IO.Path.Combine(currentDirectoryName, currentFileName + "_Temp.txt")  

As Mat's Mug also pointed out you need to extract some of the responsibilities to separate methods.

So let us introduce a method which takes a filename as input parameter and returns an array of Strings representing the content of the file.

Private Function GetFileContent(fileName As String) As String()
    Return System.IO.File.ReadAllLines(fileName)
End Function  

next we introduce a function which takes a filename as input parameter and returns wether the first character of the first line is the "|" . We first check if the read line is maybe Nothing by using String.IsNullOrWhiteSpace(). This prevents an exception if the file would be empty.

Private Function FirstLineStartsWithPrefix(fileName As String, prefix As String) As Boolean
    Using reader As New System.IO.StringReader(fileName)
        Dim line As String = reader.ReadLine()
        Return Not String.IsNullOrWhiteSpace(line) AndAlso line.StartsWith(prefix)
    End Using
End Function

Next we introduce a method which takes a String array as input parameter and returns the prefixed content. We are using the Append() and AppendLine() method. Both methods are returning a StringBuilder object, so we can use it in a fluent way.

Private Function AddPrefixToLines(content As String(), prefix As String) As String
    Dim sb As New System.Text.StringBuilder(1024)
    For Each line As String In content
        sb.Append(prefix).AppendLine(line)
    Next
    Return sb.ToString()
End Function

Next we introduce a method which takes a FileInfo object as input parameter and returns nothing (a Sub).

Private Const pipePrefix As String = "|"
Private Const fileNamePostFix As String = "_Temp.txt"
Private Sub ProcessFile(currentFileInfo As FileInfo)

    Dim fullName As String = currentFileInfo.FullName

    If Not FirstLineStartsWithPrefix(fullName, pipePrefix) Then Exit Sub

    Dim fileContent As String() = GetFileContent(currentFileInfo.FullName)

    Dim prefixedContent As String = AddPrefixToLines(fileContent, pipePrefix)

    Dim pureFileName As String = Path.GetFileNameWithoutExtension(fullName)

    Dim newFileName As String = Path.Combine(currentFileInfo.DirectoryName, pureFileName & fileNamePostFix)

    File.WriteAllText(newFileName, prefixedContent)

    currentFileInfo.Delete()

    File.Move(newFileName, fullName)

End Sub  

Now your initial code will look like

Dim fileList As FileInfo() = dirList.GetFiles

For Each fiName In fileList

    ProcessFile fiName

Next
\$\endgroup\$
  • \$\begingroup\$ Got sidetracked with other projects but returning to this finally. For your suggestion on the method to read all lines into an array, I switched the StringReader to a StreamReader because at that point the file hasn't been read into the array. I also broke up the FirstLineStartsWithPrefix function because it was returning false: Dim fileEmpty As Boolean = String.IsNullOrWhiteSpace(line) If fileEmpty = True Then Return False Else Return Not line.StartsWith(prefix) End If \$\endgroup\$ – Dave Jan 2 '15 at 19:31
4
\$\begingroup\$

Both StreamReader and StreamWriter classes inherit the Dispose method from their base class, which implements the IDisposable interface; you should always dispose of disposables - and doing so makes calling the Close method redundant. It's good that you're closing your streamreader/writer, but it's better to dispose them.

VB has a Using statement specifically for that purpose, so you don't even need to explicitly call Dispose yourself - see MSDN.

Your code has too many responsibilities. I would extract a method out of the loop's body.

The names shouldn't be prefixed with that s or any other prefix that stands for the identifier's type - that's Hungarian Notation, and heavily discouraged in any .net language.

I like that you are using the .net constants and methods instead of the legacy (/VB6) ones in the Microsoft.VisualBasic namespace.

\$\endgroup\$

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.