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

• cheezsteak: Just a readability preference, but your code window has a horizontal scroll bar. I would insert some _s. I know they are ugly but so is a 176 character line.
• Mathieu Guindon: @ptwales that's a valid point! Don't hesitate to throw in an answer/review (and include that point) if there's anything else to add :)
• AHeyne: Again an old discussion digged out by me, but I need to mention that one should honestly pay attention to the Class_Terminate() events.
• AHeyne: (sorry, lunch took too long, so I couldn't edit anymore) It es absolutely necessary to implement an error handler in there, because if any runtime error will be raised in this procedure, it is not possible to bubble it up! Instead VBA will stop execution.

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

• Mathieu Guindon: Dim rw As New RandomReaderWriter? ...good question...
• Mathieu Guindon: You're right about Dim writer As New FileWriter, there's no reason why the Dim and the Set cannot be written as a single instruction.
• RubberDuck: Yeah. The only reason not to is if you were going to declare it several lines before using it.
• Mathieu Guindon: Blast from the past: As New is actually quite dangerous, as it changes how VB handles the object's lifetime: you can't set it to Nothing, for one - Rubberduck (the VBE add-in) issues a warning for that.

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

• Mathieu Guindon: Actually if I wrote this today, the File class would not even need to be instantiated - I'd make it a "static class", with a VB_PredeclaredId attribute, making File.CreateReader usable directly.
• ya_dimon: for more files funcs see access-codelib
• RubberDuck: @Mat'sMug IIRC this code closes connections when the class goes out of scope, so using a default instance would cause big bad file locking bugs.
• AHeyne: @RubberDuck: Are you sure about this? In my opition, because the used variables for FileReader and FileWriter are in procedure scope only, they are out of scope after exiting the procedures and thus the resources are released immediately. Or am I wrong?
• AHeyne: I have now checked this with Sysintenals 'Handle': The files will be released correctly.