Versioning From rzf

Code Critic round 2. rzf submitted this code. I tried to indent it like I would, so don’t rail him if you don’t like the indents, it’s my fault.

A couple of notes: I’ve enabled HTML in my comments. If you want to post code there, you can with a little more control over how you want it to look. If you want your code critiqued, it’s still probably best to email it to me.

Jan suggested that I present a problem for Code Critic and have readers supply the code. I thinks that’s a good idea. If nobody sends code to be criticized, I’ll do it, otherwise I’ll post what you send me.

Private Sub Workbook_Open()
    Const sngVer As Single = 2003
    ‘Check for new version
    Application.ScreenUpdating = False
    Application.DisplayAlerts = False
    Application.EnableCancelKey = xlDisabled
    On Error Resume Next
    ‘open versioning file on network and save to c:
    Workbooks.Open Filename:=”G:deptsprovrembprovreimversions.txt”
    If Err.Number <> 1004 Then ActiveWorkbook.SaveAs Filename:=”c:program filesmicrosoft officeofficeversions.txt”
    If Err.Number = 1004 Then ‘not on network, used saved file
        Workbooks.Open Filename:=”c:program filesmicrosoft officeofficeversions.txt”
    End If
    If Err.Number <> 1004 Then
        If Range(“B12?) <> sngVer Then
            If MsgBox(“Version ” & Range(“B12?) & ” is now available.” & vbCrLf & “You are currently running version ” & sngVer & “.” & vbCrLf & vbCrLf & “You will need to update to the newest version to make sure you have the most current bug fixes, formatting changes, and revised forms.” & vbCrLf & vbCrLf & “If, for some odd reason, you need to use this old version then click “”YES”” below. Note, though, that no further support is available for this version”, vbInformation + vbYesNo + vbDefaultButton2, “Update Available”) = vbNo Then
                Workbooks(“versions.txt”).Close SAVECHANGES:=False
                ActiveWorkbook.Close SAVECHANGES:=False
                Application.EnableCancelKey = xlInterrupt
                Exit Sub
            End If
        End If
    Else ‘couldn’t open either file (this should never happen, but just in case)
        MsgBox “The version number could not be verified. Please be sure you are using the most current version before continuing.”, vbInformation, “Version Check Error”
    End If
    Workbooks(“versions.txt”).Close SAVECHANGES:=False
    Application.EnableCancelKey = xlInterrupt
    Application.ScreenUpdating = False
    ActiveWindow.Zoom = True
End Sub

Here are my comments:
Your lines are too long. I post in the newsgroups a lot so I have this bad habit of making my lines too short. But yours are incredibly long. I don’t know how you maintain that. Anyway, I would put my MsgBox messages in variables and use some line continuation characters so I could read and edit them without scrolling.

In your second If block, you use an unqualified Range reference (Range(“B12?)). I’m sure this works for you, but I’m not sure how. With this sub in the ThisWorkbook module, it seems that B12 would be on the ActiveSheet of ThisWorkbook, not the ActiveWorkbook. I’m sure it’s just one of those default object references that I don’t completely get, but you should consider qualifying that reference.

If I were writing this, I would use a Workbook variable and test it for Nothing instead of using the err numbers.

Set oVerWb = Workbooks.Open(Filename:=sNetPath & sFleNm)
If oVerWb Is Nothing Then
    Set oVerWb = Workbooks.Open(Filename:=sLocPath & sFleNm)
End If
If Not oVerWb Is Nothing Then
    With oVerWb
        .SaveAs sLocPath & sFleNm
            If .Sheets(1).Range(“B12?).Value <> sngVer Then

Posted in Uncategorized

6 thoughts on “Versioning From rzf

  1. While seconding the comments above, my main concern was that I couldn’t, in ten minutes or so of slightly distracted examination, get any confidence that I knew what the code was trying to do.

    These days, even a routine that (just!) fits on my screen seems too big. I could imagine this being much easier to understand if Workbook_Open() called four or five subroutines, named so as to show me what the processing steps are. It would get that crazy MsgBox text tucked away, for one thing!

    The local file path is repeated: I’d definitely want to see that held in a constant. Ditto that “magic” 1004 error code. While I was removing those duplicated values I’d probably create Consts for the other literals.

    Unless there’s a pressing reason, I’d be inclined to add a reference to Microsoft Scripting Runtime (scrrun.dll) and use FileSystemObject.FileExists(). That way I’m going to be able to get rid of the whole 1004 business altogether, which can’t be bad.

    I am a complete sucker for this kind of stuff…;-)

  2. “add a reference to Microsoft Scripting Runtime “

    Good one Mike. He could just use the Dir function as well to see if the file is there on the network.

  3. The Range(“B12?) works because when you open a workbook, unless its set to be an addin (IsAddin = True), it becomes the activeworkbook… but yes, I agree with Dick, unqualified references are a risk.

    Also, handling the workbook with a variable makes it easier to close/save/whatever to it…

    I’d also remove ANY paths (well, ok, except the network one…)… let Excel figure that out, i.e, use Application.Path to return the installed path of Office.

  4. What is the task being carried out? To omit the purpose of the program seems to me to be a major sin. I shouldn’t have to read the comments or print statements to find out what is going on.

  5. Juan:

    “it becomes the activeworkbook”

    Okay, so in the ThisWorkbook module, an unqualified sheet reference defaults to ThisWorkbook, but an unqualified Range reference defaults to the ActiveSheet of the ActiveWorkbook. I get it now, but I don’t like it.

  6. Dick,

    I like your method more than Mike’s because someone else may have to maintain this code and, even with just a basic knowledge of VBA, could figure out the “Is Nothing” test using just the help files. The scrrun.dll reference would probably make him think it’s VBA voodoo and not even attempt to understand it.

    Putting the msgbox text into a variable is a definite change for the positive that I’ll want to implement across most of my modules.

    The unqualified reference, I know, is clearly the ugly side of the module. The reason being that the versions file is a tab delimited text file. The program name becomes column A and the version becomes column B. With this being the 12th program in the list, B12 is the version number. I do not like it at all, but a .txt file is much easier to hide on the network drive than an .xls file.

    And Juan, proper commenting is more difficult than I ever imagined. It’s a fine line between too vague and way too much.

    I’d really like to see a totally different take on this. Maybe registry keys would be the way to go?

Posting code? Use <pre> tags for VBA and <code> tags for inline.

Leave a Reply

Your email address will not be published.