15
\$\begingroup\$

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 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
\$\endgroup\$
  • 3
    \$\begingroup\$ 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. \$\endgroup\$ – cheezsteak Jun 3 '14 at 18:28
  • \$\begingroup\$ @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 :) \$\endgroup\$ – Mathieu Guindon Jun 3 '14 at 18:52
  • \$\begingroup\$ Again an old discussion digged out by me, but I need to mention that one should honestly pay attention to the Class_Terminate() events. \$\endgroup\$ – Unhandled Exception Sep 22 '17 at 11:20
  • \$\begingroup\$ (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. \$\endgroup\$ – Unhandled Exception Sep 22 '17 at 11:45
12
\$\begingroup\$

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
\$\endgroup\$
  • 1
    \$\begingroup\$ Dim rw As New RandomReaderWriter? ...good question... \$\endgroup\$ – Mathieu Guindon Jun 3 '14 at 0:23
  • \$\begingroup\$ 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. \$\endgroup\$ – Mathieu Guindon Jun 3 '14 at 4:15
  • \$\begingroup\$ Yeah. The only reason not to is if you were going to declare it several lines before using it. \$\endgroup\$ – RubberDuck Jun 3 '14 at 10:53
  • \$\begingroup\$ 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. \$\endgroup\$ – Mathieu Guindon Sep 22 '17 at 12:59
6
\$\begingroup\$

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

\$\endgroup\$
  • 2
    \$\begingroup\$ 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. \$\endgroup\$ – Mathieu Guindon Jan 14 '15 at 11:58
  • \$\begingroup\$ for more files funcs see access-codelib \$\endgroup\$ – ya_dimon Jan 14 '15 at 13:10
  • 1
    \$\begingroup\$ @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. \$\endgroup\$ – RubberDuck Mar 1 '15 at 11:04
  • \$\begingroup\$ @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? \$\endgroup\$ – Unhandled Exception Sep 19 '17 at 14:39
  • \$\begingroup\$ I have now checked this with Sysintenals 'Handle': The files will be released correctly. \$\endgroup\$ – Unhandled Exception Sep 19 '17 at 14:52

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.