HiveBrain v1.2.0
Get Started
← Back to all entries
debugMinor

FileWriter supporting writing to multiple files

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
writingfilewriterfilesmultiplesupporting

Problem

I got bored with opening and closing files whenever I need to write to one, so I wrote a FileWriter class that's used like this:

Public Sub TestWriter()
    On Error GoTo ErrHandler

    Dim writer As New FileWriter
    If writer.OpenFile("c:\test.txt") Then
        writer.WriteLine "foo"
    End If

ErrHandler:
    If Err.Number <> 0 Then
        Debug.Print Err.Description
    End If
End Sub


If I change the writer.WriteLine "foo" call to writer.WriteLine "foo", "somefile.txt", I'll catch an error and output Invalid filename., as expected. I can also open both c:\test.txt and somefile.txt and then specify either file's filename when calling WriteLine, and write to either file.

I don't need to close anything, but if I want to close an opened file, I can call writer.CloseFile "c:\test.txt" and then I'll get an Invalid filename error if I try to write to it afterwards.

I don't need to explicitly anything, because any file the writer opens gets closed when the writer instance gets terminated / when the object goes out of scope.

Here's the code, I'm particularly interested in the error handling:

'expose raised errors to clients:
Public Enum FileWriterError
    InvalidFileName = vbObjectError + 42
End Enum

'manage opened files in a Dictionary:
Private openedFiles As Dictionary

'skip dictionary lookup if only 1 file is opened:
Private quickWriteFile As Long

Option Explicit


The Dictionary being used here is just a toy of mine, reviewable here. There are things I like less with that Dictionary (the fact that its Item getter gives me a KeyValuePair, for instance), but I'd like that to be outside the scope of this review.

Opening a file:

```
Public Property Get OpenedFilesCount() As Integer
OpenedFilesCount = openedFiles.Count
End Property

Public Function OpenFile(ByVal fileName As String, Optional ByVal overwrite As Boolean = False) As Boolean

Dim fileNumber As Long
fileNumber = GetFileNumber(fileName)

Solution

Using Catch: as a label for On Error GoTo is a bad, misleading idea. Error handling in vb6/vba has nothing to do with exception handling (try/catch). An error handling subroutine is typically best called ErrHandler.

The thing could be DRY'ed up a bit, CanWrite is getting a file number, and that functionality has already been extracted into GetFileNumber:

Private Function CanWrite(ByVal fileName As String, ByRef outFileNumber As Long) As Boolean
    Dim result As Boolean
    Dim fileNumber As Long
    Dim proceed As Boolean

    If quickWriteFile <> 0 And fileName = vbNullString Then
        fileNumber = quickWriteFile
        CanWrite = True
    Else
        CanWrite = openedFiles.TryGetValue(fileName, fileNumber)
    End If

    outFileNumber = fileNumber

End Function


The function should be written to use GetFileNumber instead of reimplementing the same logic:

Private Function CanWrite(ByVal fileName As String, ByRef outFileNumber As Long) As Boolean

    outFileNumber = GetFileNumber(fileName)
    If outFileNumber = FreeFile Then
        outFileNumber = 0
    Else
        CanWrite = True
    End If

End Function


WriteLine is removing the fileNumber from openedFiles when there's an error, but it's not clear that the file number in question refers to a closed file at that point, and then removing the file number from openedFiles will cause the file to remain opened when the class terminates, or even if client code explicitly calls CloseFile with the file name being used here.

Catch:
    If Err.number <> 0 Then
        result = False
        openedFiles.Remove fileNumber
        Err.Raise Err.number, Err.source, Err.description, Err.HelpFile, Err.HelpContext
    End If

End Sub


Removing items from openedFiles should be the job of the CloseFile method; if the error handler was written like this:

Catch:
    If Err.number <> 0 Then
        result = False
        CloseFile fileName
        Err.Raise Err.number, Err.source, Err.description, Err.HelpFile, Err.HelpContext
    End If


Now this is rather inefficient, because you already have a fileNumber, and CloseFile is going to get it again. Perhaps a private CloseFileNumber (or CloseInternal) method taking a file number instead of a file name could be useful here; CloseFile could then call this CloseInternal method after resolving the fileNumber from the given fileName.

Code Snippets

Private Function CanWrite(ByVal fileName As String, ByRef outFileNumber As Long) As Boolean
    Dim result As Boolean
    Dim fileNumber As Long
    Dim proceed As Boolean

    If quickWriteFile <> 0 And fileName = vbNullString Then
        fileNumber = quickWriteFile
        CanWrite = True
    Else
        CanWrite = openedFiles.TryGetValue(fileName, fileNumber)
    End If

    outFileNumber = fileNumber

End Function
Private Function CanWrite(ByVal fileName As String, ByRef outFileNumber As Long) As Boolean

    outFileNumber = GetFileNumber(fileName)
    If outFileNumber = FreeFile Then
        outFileNumber = 0
    Else
        CanWrite = True
    End If

End Function
Catch:
    If Err.number <> 0 Then
        result = False
        openedFiles.Remove fileNumber
        Err.Raise Err.number, Err.source, Err.description, Err.HelpFile, Err.HelpContext
    End If

End Sub
Catch:
    If Err.number <> 0 Then
        result = False
        CloseFile fileName
        Err.Raise Err.number, Err.source, Err.description, Err.HelpFile, Err.HelpContext
    End If

Context

StackExchange Code Review Q#52185, answer score: 6

Revisions (0)

No revisions yet.