File reader/writer (text)

Question

Following-up on this post, I wanted to be able to use my FileWriter with a syntax reminiscent of .net's using blocks, which ensure proper disposal of resources (i.e. closing the file/stream).

I'm looking for general feedback on the error handling and the overall cleanliness and readability of the code.

What I have can be successfully used like this:

Dim file As New file
Dim path As String
path = "c:\test.txt"

With file.CreateWriter(path)
    .AppendLine "foo"
    .AppendLine "bar"
    .AppendLine "foobar"
End With

Dim data As String
With file.CreateReader(path)
    data = .ReadToEnd
End With
Debug.Print data

Which is exactly what I wanted, and possibly as close to a using block that vb6 can get. This required implementing a small File class:

File Class Module

Option Explicit

Public Function CreateWriter(ByVal path As String, Optional ByVal overwrite As Boolean = True) As FileWriter
    Dim writer As FileWriter
    Set writer = New FileWriter
    If writer.OpenFile(path, overwrite) Then Set CreateWriter = writer
End Function

Public Function CreateReader(ByVal path As String) As FileReader
    Dim reader As FileReader
    Set reader = New FileReader
    If reader.OpenFile(path) Then Set CreateReader = reader
End Function

Public Function Exists(ByVal path As String) As Boolean
    Exists = (Dir(path) <> vbNullString)
End Function

I plan to eventually add members to this class, so that it becomes some kind of general-purpose "file helper"; the Exists method was added to that effect, and more will be added when I think of something else that would be useful to have there.

FileReader Class Module

Private Const moduleErrorBase As Long = &HBEEF

Public Enum FileReaderError
    FileNotOpened = vbObjectError + moduleErrorBase + 42
    FileAlreadyOpened
End Enum

Private openedFileNumber As Integer
Private openedFileName As String

Private bofFlag As Boolean
Option Explicit

Public Function ReadToEnd() As String
Const method As String = "ReadToEnd"
On Error GoTo ErrHandler

    Dim result As String
    Dim data As String

    If Not bofFlag Then
        'file was partially read, output will be the remainder of the file
        'warn? raise error?
    End If

    While Not EOF(openedFileNumber)
        Line Input #openedFileNumber, data
        result = result & data & vbNewLine
    Wend

    CloseFile
    ReadToEnd = result

CleanExit:
    Exit Function

ErrHandler:
    Err.Raise Err.Number, GetErrorSource(method), Err.Description, Err.HelpFile, Err.HelpContext
End Function

Public Function OpenFile(ByVal fileName As String) As Boolean
Const method As String = "OpenFile"
On Error GoTo ErrHandler

    If openedFileNumber <> 0 Then OnFileAlreadyOpenedError method, fileName

    openedFileNumber = FreeFile
    openedFileName = fileName

    Open openedFileName For Input As #openedFileNumber
    bofFlag = True

    OpenFile = True

CleanExit:
    Exit Function

ErrHandler:
    Err.Raise Err.Number, GetErrorSource(method), Err.Description
End Function

Public Function ReadLine(ByRef data As String) As Boolean
Const method As String = "ReadLine"
On Error GoTo ErrHandler

    If openedFileNumber = 0 Then OnFileNotOpenedError method

    If EOF(openedFileNumber) Then
        ReadLine = False
        Exit Function
    End If

    Line Input #openedFileNumber, data
    bofFlag = False

CleanExit:
    Exit Function

ErrHandler:
    Err.Raise Err.Number, GetErrorSource(method), Err.Description
End Function

Public Sub CloseFile()
Const method As String = "CloseFile"
On Error GoTo ErrHandler

    'If openedFileNumber = 0 Then OnFileNotOpenedError method, openedFileNumber

    Close #openedFileNumber
    openedFileNumber = 0

CleanExit:
    Exit Sub

ErrHandler:
    Err.Raise Err.Number, GetErrorSource(method), Err.Description, Err.HelpFile, Err.HelpContext
End Sub

Private Sub OnFileNotOpenedError(ByVal method As String)
    Err.Raise FileWriterError.FileNotOpened, GetErrorSource(method), "File #" & openedFileNumber & "(" & openedFileName & ") was unexpectedly closed."
End Sub

Private Sub OnFileAlreadyOpenedError(ByVal method As String, ByVal fileName As String)
    Err.Raise FileWriterError.FileAlreadyOpened, GetErrorSource(method), "File '" & fileName & "' cannot be opened with this instance at this point. A file is already opened."
End Sub

Private Function GetErrorSource(ByVal method As String) As String
    GetErrorSource = TypeName(Me) & "." & method
End Function

Private Sub Class_Terminate()
    CloseFile
End Sub

FileWriter Class Module

This code is the revised code from the previous/linked post, modified per reviews received. I have given up on supporting multiple opened files at once, since with the File class and the With syntax one gets much cleaner code, ..and it greatly cleaned up this code as well:

Private Const moduleErrorBase As Long = &HFADE

'expose raised errors to clients:
Public Enum FileWriterError
    FileNotOpened = vbObjectError + moduleErrorBase + 42
    FileAlreadyOpened
End Enum

Private openedFileName As String
Private openedFileNumber As Long
Option Explicit

Public Function OpenFile(ByVal fileName As String, Optional ByVal overwrite As Boolean = True) As Boolean
Const method As String = "OpenFile"
On Error GoTo ErrHandler

    If openedFileNumber <> 0 Then OnFileAlreadyOpenedError method, fileName
    openedFileNumber = FreeFile
    openedFileName = fileName

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

CleanExit:
    OpenFile = True
    Exit Function

ErrHandler:
    Err.Raise Err.Number, GetErrorSource(method), Err.Description, Err.HelpFile, Err.HelpContext
End Function

Public Sub AppendLine(ByVal data As String)
    AppendInternal data, True
End Sub

Public Sub Append(ByVal data As String)
    AppendInternal data, False
End Sub

Private Sub AppendInternal(ByVal data As String, ByVal withLineFeed As Boolean)
Const method As String = "AppendInternal"
On Error GoTo ErrHandler

    If openedFileNumber = 0 Then OnFileNotOpenedError method

    If withLineFeed Then
        Print #openedFileNumber, data
    Else
        Print #openedFileNumber, data;
    End If

CleanExit:
    Exit Sub

ErrHandler:

    'handle "52: Bad file name or number" by raising a FileWriterError.FileNotOpened instead:
    If Err.Number = 52 Then OnFileNotOpenedError method

    'close file it *any* error occurs writing to it:
    CloseFile

    'bubble up all errors
    Err.Raise Err.Number, Err.source, Err.Description
End Sub

Private Sub OnFileNotOpenedError(ByVal method As String)
    Err.Raise FileWriterError.FileNotOpened, GetErrorSource(method), "File #" & openedFileNumber & "(" & openedFileName & ") was unexpectedly closed."
End Sub

Private Sub OnFileAlreadyOpenedError(ByVal method As String, ByVal fileName As String)
    Err.Raise FileWriterError.FileAlreadyOpened, GetErrorSource(method), "File '" & fileName & "' cannot be opened with this instance at this point. A file is already opened."
End Sub

Public Sub CloseFile()
Const method As String = "CloseFile"
On Error GoTo ErrHandler

    'If openedFileNumber = 0 Then OnFileNotOpenedError method, openedFileNumber

    Close #openedFileNumber
    openedFileNumber = 0

CleanExit:
    Exit Sub

ErrHandler:
    Err.Raise Err.Number, GetErrorSource(method), Err.Description, Err.HelpFile, Err.HelpContext
End Sub

Private Function GetErrorSource(ByVal method As String) As String
    GetErrorSource = TypeName(Me) & "." & method
End Function

Private Sub Class_Terminate()
    CloseFile
End Sub

Answer 1

Error Handling

Your error handlers look much cleaner (and ultimately safer) than before. I also like your GetErrorSoure() routine and the "CleanExit:" name of the labels. Very concise. (<-Read as, "I'll be 'borrowing' more of your code'). I see a small issue in your OpenFile() routine. You should probably set OpenFile = False prior to re-raising the error. I know it's implicitly set to False, but it's good to leave it for the maintainer.

CleanExit:
    OpenFile = True
    Exit Function

ErrHandler:
    OpenFile = False
    Err.Raise Err.Number, GetErrorSource(method), Err.Description, Err.HelpFile, Err.HelpContext
End Function

That's extremely nit-picky though. They look good.

And this,

If Not bofFlag Then
    'file was partially read, output will be the remainder of the file
    'warn? raise error?
End If

go ahead and raise it. Let the coder decide what to do if it happens at a higher level.

Readability

I'm a fan of writing one line if statements, but you should probably bite the bullet and separate them out onto separate lines. Mr. Maintainer will thank you.

I'm conflicted about my next piece of advise. From a readability standpoint, it is correct to do this.

Dim writer As FileWriter
Set writer = New FileWriter

It is easier to read, but it's also an utter waste of effort because you immediately use the FileWriter. Given I don't like it because it's a waste of effort, there's no sense in changing it.

This is good. I like how you have the ability to use the with statement, but it feels weird to see it with a method. It's technically correct to use the verb-noun naming. It just feels weird. I keep wanting to just call it "Writer".

With file.CreateWriter(path)
    .AppendLine "foo"
    .AppendLine "bar"
    .AppendLine "foobar"
End With

Ok. I'm done nitpicking. It looks really good as far as I can tell. Just some food for thought now.

What if you wanted to extend this to allow random file io? How would you handle that?

---

I finally figured out how I would "fix" this.

With file.CreateWriter(path)
    .AppendLine "foo"
    .AppendLine "bar"
    .AppendLine "foobar"
End With

CreateWriter returns a FileWriter. For readability, I would want to call it like With writer, but this does mean using an extra variable.

Dim writer As FileWriter
Set writer = file.CreateWriter(path)

With writer
    .AppendLine "foo"
    .AppendLine "bar"
    .AppendLine "foobar"
End With

Answer 2

Error Handling:

• By ErrHandler you don't need to pass default args after Err.number and custom args:

  ErrHandler:
  Err.Raise Err.Number, GetErrorSource(method)
  

  should be enough

• I like your GetErrorSource() method.

• I like your error numbers moduleErrorBase, enum etc.

Naming Conventions:

In VBA, it's nice to have vars, args, subs, functions, and methods with same style (a capital letter starting is the default) because VBA doesn't know different name cases. So if you have lowercase name, everything else with the same name will be lowercase too (very bad for version control).

For instance, someone using Writer() as property of some class. Your code rewrite the property name to lower case. You can use some prefix, to reduce the probability for name collisions, like Dim fwWriter and to improve readability

For your comment about "Dim and the Set as a single instruction":

There is reason to make 2 lines, which is its performance:

Never Dim anything As New if you're concerned about speed. VB6 will treat such a variable as an auto-instantiated variable. Every time you use it, VB will check if it should be instantiated. This will cost you some extra CPU cycles.

aivosto