9
\$\begingroup\$

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)

    'guard against opening a file that's already opened:
    If fileNumber <> FreeFile Then
        OpenFile = True
        Exit Function
    End If

    On Error GoTo Catch

    If overwrite Or Dir(fileName) = vbNullString Then
        Open fileName For Output As #fileNumber
    Else
        Open fileName For Append As #fileNumber
    End If

    openedFiles.Add fileName, fileNumber
    quickWriteFile = IIf(openedFiles.Count = 1, fileNumber, 0)

Catch:
    If Err.number <> 0 Then
        Err.Clear
    End If

    OpenFile = (openedFiles.ContainsKey(fileName))

End Function

Writing a line of text into a file:

Public Sub WriteLine(ByVal data As String, Optional ByVal fileName As String = vbNullString)
    Dim fileNumber As Long
    Dim result As Boolean

    On Error GoTo Catch

    If CanWrite(fileName, fileNumber) Then
        Print #fileNumber, data
        result = True
    Else
        Err.Raise FileWriterError.InvalidFileName, TypeName(Me) & ".WriteLine", "Invalid filename."
    End If

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

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

Closing files:

Public Sub CloseFile(Optional ByVal fileName As String = vbNullString)
    If openedFiles.Count = 0 Then Exit Sub

    Dim fileNumber As Long
    fileNumber = GetFileNumber(fileName)

    If fileNumber <> FreeFile Then
        Close #fileNumber
        openedFiles.Remove fileNumber
        If fileNumber = quickWriteFile Then quickWriteFile = 0
    End If

    If openedFiles.Count = 1 Then quickWriteFile = openedFiles.Values.First

End Sub

Public Sub CloseAllFiles()
    Dim file As KeyValuePair
    For Each file In openedFiles
        Close #file.value
    Next
    openedFiles.Clear
    quickWriteFile = 0
End Sub

Private Function GetFileNumber(ByVal fileName As String) As Long
    Dim result As Long

    If quickWriteFile <> 0 And fileName = vbNullString Then
        result = quickWriteFile

    ElseIf Not openedFiles.TryGetValue(fileName, result) Then
        result = FreeFile

    End If

    GetFileNumber = result
End Function

Class initialization and termination:

Private Sub Class_Initialize()
    Set openedFiles = New Dictionary
End Sub

Private Sub Class_Terminate()
    CloseAllFiles
    Set openedFiles = Nothing
End Sub
\$\endgroup\$
5
\$\begingroup\$

I'll start by saying that I like this. I like it a lot and I'll be doing this from now on.

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

But, if you plan on raising more than one error, you should use a constant to give your class a range of error codes like this:

Private Const ErrorBaseNumber as Integer = 1000

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

Here SomeOtherError is implicitly set to vbObjectError + ErrorBaseNumber + 43. Ideally, you keep track of your base numbers somewhere and give all of your classes different base numbers.

I'd like to address your OpenFile() function a little bit. Where you've declared On Error GoTo Catch is a little late in your procedure. If for some (unlikely reason) you get an error before you've declared it, it won't be trapped. It should go directly after your declaration.

Public Function OpenFile(ByVal fileName As String, Optional ByVal overwrite As Boolean = False) As Boolean
On Error GoTo Catch

And do you really mean to Catch an error just to clear it? This is no better than using the On Error Resume Next statement. (Which should never be used. Never.)

You seem to be trying to implement a Try...Catch...Finally structure. That's not how it should be done in VBA/VB6. VBA/VB6 uses an "Always Exit" paradigm. In a perfect world, your code should never get to the End Function statement.

Here's a pretty standard pattern for error handling in VBA.

PublicFunction OpenFile(ByVal fileName as String, Optional ByVal overwrite as Boolean = False) As Boolean
On Error GoTo ErrHandler

    Dim fileNumber As Long
    fileNumber = GetFileNumber(fileName)

    'guard against opening a file that's already opened:
    If fileNumber <> FreeFile Then
        OpenFile = True
        Exit Function
    End If

    If overwrite Or Dir(fileName) = vbNullString Then
        Open fileName For Output As #fileNumber
    Else
        Open fileName For Append As #fileNumber
    End If

    openedFiles.Add fileName, fileNumber
    quickWriteFile = IIf(openedFiles.Count = 1, fileNumber, 0)

ExitFunction:
    ' clean up (if there was anything to clean up)
    Exit Function

ErrHandler:
    ' Let's report the error to the user. Decide for yourself what to do with the error. 
    ' Just do something with it.
    MsgBox "Unexpected Error #" & Err.Number & " occurred." & vbCrLf & _
        "Error Source: " & Err.Source & vbCrLf & _
        "Error Procedure: OpenFile" & vbCrLF & _
        "Error Description: " & Err.Description
    Resume ExitFunction
End Function

In the Writeline Sub, I do like the way you raise errors, but it could lie to you if you for some reason change the name of the Sub, but forget to update the Function Name.

Err.Raise FileWriterError.InvalidFileName, TypeName(Me) & ".WriteLine", "Invalid filename."

Should be

Const ProcName As String = "WriteLine"
Err.Raise FileWriterError.InvalidFileName, TypeName(Me) & "." & ProcName, "Invalid FileName."

Before you start screaming "How is that better?", please read this answer of mine on Stack Overflow. The idea is to leverage the Microsoft Visual Basic for Applications Extensibility library to automate the maintenance of those specifically named constants prior to each build.

It would also be good practice to tie the "Invalid FileName" string to the FileWriterError.InvalidFileName enum via a private function.

Private Function getErrorMessage(ErrorNumber as FileWriterError)
    Select Case ErrorNumber
        Case InvalidFileName : getErrorMessage = "Invalid File Name."
        Case SomeOtherError : getErrorMessage = "Some other error happened."
    End Select
End Function

Blast me for the colon later, this is your code review, not mine =;)-

TL;DR

Don't try to use Try...Catch...Finally block in VBA/VB6. The language isn't built that way. Your subs should always exit, not end.

Additional Reading from Microsoft on VBA Error Handling

\$\endgroup\$
6
\$\begingroup\$

Using Catch: as a label for On Error GoTo is a bad, misleading idea. Error handling in / 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.

\$\endgroup\$
5
\$\begingroup\$

Given that FileWriter works solely with files, you can drop File from the public method names leaving Open and Close, assuming these aren't reserved in .

It looks like the built-in Print automatically adds a newline unless you specify the new character position afterwards. If that's easy, e.g. without knowing how long the file currently is, it might be nice to add a Write method that does so.

\$\endgroup\$
  • \$\begingroup\$ I actually intended to call them Open and Close, but they're indeed reserved keywords, so I added this File suffix - I agree it's redundant though, and I'll +1 for adding a Write method (WriteToFile? ...Write is reserved as well!) when votes reload ;) \$\endgroup\$ – Mathieu Guindon Jun 1 '14 at 18:21
  • 1
    \$\begingroup\$ The best I can come up with so far is WriteText or WriteString. You need something to counter Line. WriteToFile would require changing the other to WriteLineToFile to maintain symmetry. :) \$\endgroup\$ – David Harkness Jun 1 '14 at 18:28
  • 1
    \$\begingroup\$ How about WriteLine (with a line feed) and Append (without a line feed)? If withLineFeed Then Print #fileNumber, data Else Print #fileNumber, data; End If - the ; prevents writing the line feed, so I've implemented a WriteInternal private method that takes a withLineFeed Boolean, and WriteLine simply goes WriteInternal data, True, fileName while Append goes WriteInternal data, False, fileName. Pretty neat! \$\endgroup\$ – Mathieu Guindon Jun 1 '14 at 19:06
  • 1
    \$\begingroup\$ That might imply that WriteLine starts from the beginning. How about Append and AppendLine? I know, WriteLine is so nice, and it's a FileWriter after all. Naming is hard! \$\endgroup\$ – David Harkness Jun 1 '14 at 19:34
1
\$\begingroup\$

In response to RubberDucks getErrorMessage procedure, I got the idea to use Choose (I was right now thinking about a possible use case of it) to make it looking more similar to the error enumeration and thus in my opinion easier to maintain:

Private Const ERROR_BASE As Long = &H100

Public Enum SystemError
  CustomErrorA = vbObjectError + ERROR_BASE
  CustomErrorB
  CustomErrorC
End Enum

Private Function GetErrorMessage(errorNumber As SystemError)
  GetErrorMessage = Choose(errorNumber - vbObjectError - ERROR_BASE + 1, _
    "Custom error A description", _
    "Custom error B description", _
    "Custom error C description" _
    )
End Function
\$\endgroup\$
  • \$\begingroup\$ Hah, I like that! This is the first use of Choose I see and agree with! \$\endgroup\$ – Mathieu Guindon Sep 19 '17 at 17:42

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.