Keep It Simple

We all know that coding is great fun, even code design is fun, but testing and debugging are most certainly not fun. As such, we have to do what we can to lighten that burden. 

One of my underlying principles in coding is in keeping the code well structured, well laid out, and generally easy to follow, so as to make it easier to maintain, easier to debug, and just generally a better experience.

Whilst spending some time on a forum today, I came across this code which had been found elsewhere. My question to you is, what is wrong with the following code?

 

Sub Copy_and_Rename_To_New_Folder()
     ”MUST set reference to Windows Script Host Object Model in the project using this code!
     ‘This procedure will copy all files in a folder, and insert the last modified date into the file name’
     ‘it is identical to the other procedure with the exception of the renaming…
     ‘In this example, the renaming has utilized the files Last Modified date to “tag” the copied file.
     ‘This is very useful in quickly archiving and storing daily batch files that come through with the same name on
     ‘a daily basis. Note: All files in current folder will be copied this way unless condition testing applied as in prior example.
    Dim objFSO As New Scripting.FileSystemObject, objFolder As Scripting.folder, PathExists As Boolean
    Dim objFile As Scripting.File, strSourceFolder As String, strDestFolder As String
    Dim x, Counter As Integer, Overwrite As String, strNewFileName As String
    Dim strName As String, strMid As String, strExt As String
    Dim sSavePath3 As String
    Application.ScreenUpdating = False ‘turn screenupdating off
    Application.EnableEvents = False ‘turn events off
     ‘Call Show_BrowseDirectory_Dialog ‘ Allows the Dynmaic selection of Save Path
     ‘identify path names below:
    strSourceFolder = “C:\Test” ‘Source path
     ‘strDestFolder = “C:\Test\Destination” ‘destination path, does not have to exist prior to execution
     ”””””NOTE: Path names can be strings built in code, cell references, or user form text box strings”””
     ”””””example: strSourceFolder = Range(“A1″)
     ‘below will verify that the specified destination path exists, or it will create it:
    On Error Resume Next
    x = GetAttr(strDestFolder) And 0
    If Err = 0 Then ‘if there is no error, continue below
        PathExists = True ‘if there is no error, set flag to TRUE
        Overwrite = MsgBox(“The folder may contain duplicate files,” & vbNewLine & _
        “Do you wish to overwrite existing files with same name?”, vbYesNo, “Alert!”)
         ‘message to alert that you may overwrite files of the same name since folder exists
        If Overwrite <> vbYes Then Exit Sub ‘if the user clicks YES, then exit the routine..
         ‘Else: ‘if path does NOT exist, do the next steps
         ‘ PathExists = False ‘set flag at false
         ‘ If PathExists = False Then MkDir (strDestFolder) ‘If path does not exist, make a new one
    End If ‘end the conditional testing
    On Error Goto ErrHandler
    Set objFSO = CreateObject(“Scripting.FileSystemObject”) ‘creates a new File System Object reference
    Set objFolder = objFSO.GetFolder(strSourceFolder) ‘get the folder
    Counter = 0 ‘set the counter at zero for counting files copied
    If Not objFolder.Files.Count > 0 Then Goto NoFiles ‘if no files exist in source folder “Go To” the NoFiles section
    For Each objFile In objFolder.Files ‘for every file in the folder…
         ‘parse the name in three pieces, file name middle and extension.
        strName = Left(objFile.Name, Len(objFile.Name) – 4) ‘remove extension and leave name only
         ‘strMid = Format(objFile.DateLastModified, “_mmm_dd_yy”) ‘insert and format files date modified into name
         ‘strMid = Format(Now(),”_mmm_dd_yy”) ‘sample of formatting the current date into the file name
        strExt = Right(objFile.Name, 4) ‘the original file extension
         ‘ For Valeo Daily
        Dim strDate As String
         ‘strDate = Right(strName, 8)
         ‘strNewFileName = Mid(strDate, 3, 2) & “-” & Mid(strDate, 5, 2) & “-” & Mid(strDate, 7, 2) & ” elec Valeo ” & _
        Left(strName, Len(strName) – 9) & strExt ‘build the string file name (can be done below as well)
         ‘ End Valeo Daily
         ‘strNewFileName = strName & ” TET” & strExt
        strNewFileName = “09 lqd ” & strName & ” TRS” & strExt
         ‘objFile.Copy strDestFolder & “\” & strNewFileName ‘copy the file with NEW name!
        objFile.Name = strNewFileName ‘<====this can be used to JUST RENAME, and not copy
         ‘The below line can be uncommented to MOVE the files AND rename between folders, without copying
         ‘objFile.Move strDestFolder & “\” & strNewFileName
        
         ‘End If ‘where conditional check, if applicable would be placed.
         ‘ Uncomment the If…End If Conditional as needed
        Counter = Counter + 1
    Next objFile ‘go to the next file
     ‘MsgBox “All ” & Counter & ” Files from ” & vbCrLf & vbCrLf & strSourceFolder & vbNewLine & vbNewLine & _
    ” copied/moved to: ” & vbCrLf & vbCrLf & strDestFolder, , “Completed Transfer/Copy!”
     ‘Message to user confirming completion
    Set objFile = Nothing: Set objFSO = Nothing: Set objFolder = Nothing ‘clear the objects
    Exit Sub
NoFiles:
     ‘Message to alert if Source folder has no files in it to copy
    MsgBox “There Are no files or documents in : ” & vbNewLine & vbNewLine & _
    strSourceFolder & vbNewLine & vbNewLine & “Please verify the path!”, , “Alert: No Files Found!”
    Set objFile = Nothing: Set objFSO = Nothing: Set objFolder = Nothing ‘clear the objects
    Application.ScreenUpdating = True ‘turn screenupdating back on
    Application.EnableEvents = True ‘turn events back on
    Exit Sub ‘exit sub here to avoid subsequent actions
ErrHandler:
     ‘A general error message
    MsgBox “Error: ” & Err.Number & Err.Description & vbCrLf & vbCrLf & vbCrLf & _
    “Please verify that all files in the folder are not currently open,” & _
    “and the source directory is available”
    Err.Clear ‘clear the error
    Set objFile = Nothing: Set objFSO = Nothing: Set objFolder = Nothing ‘clear the objects
    Application.ScreenUpdating = True ‘turn screenupdating back on
    Application.EnableEvents = True ‘turn events back on
End Sub
Sub FolderExists()
    Dim FSO
    Dim folder As String
    folder = “G:\Marketing\Market Price Guides\1Valeo Power Summaries”
    Set FSO = CreateObject(“Scripting.FileSystemObject”)
    If FSO.FolderExists(folder) Then
        MsgBox folder & ” is a valid folder/path.”, vbInformation, “Path Exists”
    Else
        MsgBox folder & ” is NOT a valid folder/path. “, vbInformation, ” Invalid Path”
    End If
End Sub

 

That is a rhetorical question as I will tell you what is wrong with it. It is over-commented that is what is wrong with it, grossly over-commented.

Even allowing for the fact that many of the comments were probably added because it was being posted as a response in an Excel forum, they are totally self-defeating to my mind.

 Let’s look at in detail …

 

Sub Copy_and_Rename_To_New_Folder()
”MUST set reference to Windows Script Host Object Model in the project using this code!
‘This procedure will copy all files in a folder, and insert the last modified date into the file name’
‘it is identical to the other procedure with the exception of the renaming…
‘In this example, the renaming has utilized the files Last Modified date to “tag” the copied file.
‘This is very useful in quickly archiving and storing daily batch files that come through with the same name on
‘a daily basis. Note: All files in current folder will be copied this way unless condition testing applied as in prior example.

 

A relatively standard practice, say what it does. But what a lot of words to say it, many of which I feel could have been dispensed with a meaningful procedure name.

 The library reference comment may be the only bit of this I find useful, but even that is relatively obvious from the following variable declarations.

 

    Application.ScreenUpdating = False ‘turn screenupdating off
   
Application.EnableEvents = False ‘turn events off

 

The code says it all, no need for any comments here.

 

     ‘Call Show_BrowseDirectory_Dialog ‘ Allows the Dynmaic selection of Save Path
     ‘identify path names below:

 

Presumably, this is some old version  code … so remove it.

 

    strSourceFolder = “C:\Test” ‘Source path

 

The name of the variable tells you all you need to know.

 

     ‘strDestFolder = “C:\Test\Destination” ‘destination path, does not have to exist prior to execution
     ”””””NOTE: Path names can be strings built in code, cell references, or user form text box strings”””
     ”””””example: strSourceFolder = Range(“A1″)
     ‘below will verify that the specified destination path exists, or it will create it:

 

Old code again, but even here what does the comments within say, it explained nothing to me

 

    On Error Resume Next
   
x = GetAttr(strDestFolder) And 0
    If Err = 0 Then ‘if there is no error, continue below

 

This is obvious, , no need for any comments here.

 

        PathExists = True ‘if there is no error, set flag to TRUE

 

The code is clear, no need for any comments here. The only comment that would help IMO is an explanation of what PathExists is used for, but the name tells you that.

 

        Overwrite = MsgBox(“The folder may contain duplicate files,” & vbNewLine & _
        “Do you wish to overwrite existing files with same name?”, vbYesNo, “Alert!”)
         ‘message to alert that you may overwrite files of the same name since folder exists

 

Good idea, add a  comment that essentially repeats the message.

 

        If Overwrite <> vbYes Then Exit Sub ‘if the user clicks YES, then exit the routine..

 

 

Totally pointless comment.

 

         ‘Else: ‘if path does NOT exist, do the next steps
        
‘ PathExists = False ‘set flag at false
         ‘ If PathExists = False Then MkDir (strDestFolder) ‘If path does not exist, make a new one

 

Old code, but again with obvious comments.

 

    End If ‘end the conditional testing

 

Totally pointless comment.

 

    On Error Goto ErrHandler
    Set objFSO = CreateObject(“Scripting.FileSystemObject”) ‘creates a new File System Object reference

 

The code tells you that.

 

    Set objFolder = objFSO.GetFolder(strSourceFolder) ‘get the folder

 

The code tells you that.

 

    Counter = 0 ‘set the counter at zero for counting files copied

 

The code tells you that, the only news here it is a files counter, so just say that if anything.

 

    If Not objFolder.Files.Count > 0 Then Goto NoFiles ‘if no files exist in source folder “Go To” the NoFiles section

 

The code tells you that, res-state what the code says.

 

    For Each objFile In objFolder.Files ‘for every file in the folder…

 

The code tells you that, basic usage of For.

 

         ‘parse the name in three pieces, file name middle and extension.

 

Some might find this useful, I wouldn’t, the code says it.

 

        strName = Left(objFile.Name, Len(objFile.Name) – 4) ‘remove extension and leave name only

 

Anyone familiar with filenames should get this, although it would be better to use a technique that allows for variable extension types.

 

         ‘strMid = Format(objFile.DateLastModified, “_mmm_dd_yy”) ‘insert and format files date modified into name
         ‘strMid = Format(Now(),”_mmm_dd_yy”) ‘sample of formatting the current date into the file name

 

Look at that, some of the code has been commented out, rendering a previous comment incorrect.

 

        strExt = Right(objFile.Name, 4) ‘the original file extension

 

I repeat my earlier comment on this.

 

         ‘ For Valeo Daily

 

I have absolutely no idea what this means, so it only serves to confuse me.

 

        Dim strDate As String
        
‘strDate = Right(strName, 8)
         ‘strNewFileName = Mid(strDate, 3, 2) & “-” & Mid(strDate, 5, 2) & “-” & Mid(strDate, 7, 2) & ” elec Valeo ” & _
        Left(strName, Len(strName) – 9) & strExt ‘build the string file name (can be done below as well)
         ‘ End Valeo Daily
         ‘strNewFileName = strName & ” TET” & strExt
        strNewFileName = “09 lqd ” & strName & ” TRS” & strExt

 

As before, a lot of old code commented out, adding tgo the confusion, reducing the readability.

 

 

         ‘objFile.Copy strDestFolder & “\” & strNewFileName ‘copy the file with NEW name!
        objFile.Name = strNewFileName ‘<====this can be used to JUST RENAME, and not copy
         ‘The below line can be uncommented to MOVE the files AND rename between folders, without copying
         ‘objFile.Move strDestFolder & “\” & strNewFileName          
        
         ‘End If ‘where conditional check, if applicable would be placed.
         ‘ Uncomment the If…End If Conditional as needed

 

This could be useful comments, but I would assume that any decent coder could work this out if they need to do it. Since when do we add code, commented out, to cater for other situations?

 

        Counter = Counter + 1

    Next objFile ‘go to the next file

 

Totally unnecessary comment.

 

     ‘MsgBox “All ” & Counter & ” Files from ” & vbCrLf & vbCrLf & strSourceFolder & vbNewLine & vbNewLine & _
   
” copied/moved to: ” & vbCrLf & vbCrLf & strDestFolder, , “Completed Transfer/Copy!”
     ‘Message to user confirming completion

 

Old code again, presumably.

 

    Set objFile = Nothing: Set objFSO = Nothing: Set objFolder = Nothing ‘clear the objects

 

Comment only says what the code says.

 

    Exit Sub

NoFiles:
    
‘Message to alert if Source folder has no files in it to copy
    MsgBox “There Are no files or documents in : ” & vbNewLine & vbNewLine & _
    strSourceFolder & vbNewLine & vbNewLine & “Please verify the path!”, , “Alert: No Files
Found!”

 

Comment only says what the code says.

 

    Set objFile = Nothing: Set objFSO = Nothing: Set objFolder = Nothing ‘clear the objects

 

Comment only says what the code says.

 

    Application.ScreenUpdating = True ‘turn screenupdating back on
   
Application.EnableEvents = True ‘turn events back on
   
Exit Sub ‘exit sub here to avoid subsequent actions

 

The code says it all, no need for any comments here.

 

ErrHandler:
    
‘A general error messagee
    MsgBox “Error: ” & Err.Number & Err.Description & vbCrLf & vbCrLf & vbCrLf & _
    “Please verify that all files in the folder are not currently open,” & _
    “and the source directory is available”
    Err.Clear ‘clear the error
    Set objFile = Nothing: Set objFSO = Nothing: Set objFolder = Nothing ‘clear the objects

 

Comment only says what the code says.

 

    Application.ScreenUpdating = True ‘turn screenupdating back on
   
Application.EnableEvents = True ‘turn events back on

 

The code says it all, no need for any comments here.

 

End Sub
Sub FolderExists()
    Dim FSO
    Dim folder As String
    folder = “G:\Marketing\Market Price Guides\1Valeo Power Summaries”
    Set FSO = CreateObject(“Scripting.FileSystemObject”)
    If FSO.FolderExists(folder) Then
        MsgBox folder & ” is a valid folder/path.”, vbInformation, “Path Exists”
    Else
        MsgBox folder & ” is NOT a valid folder/path. “, vbInformation, ” Invalid Path”
    End If
End Sub


Now I am ready to accept that I am in a minority ( a minority of only two that I know of), but I generally find comments to be of no use, and I fully expect the standard police to be down on me for my views. The code above shows all of the bad usages of comments that I come across,

  • comments that just repeat what the code says
  • too much verbiage in the comments
  • comments that try so hard to be clear, they are incomprehensible
  • meaningless comments
  • out of date comments
and so on.

But worse of all, and my biggest gripe against comments is that they make the code so hard to read. When I am debugging, I am reading the code, I am looking back at what has happened, I am looking forward at what is about to happen, and those comments just get in the way. If they were helpful in other ways, then …, but they rarely are.

Let’s be honest, how many of us really find other people’s comments helpful, and with our own they usually only tell us what we can read from (our own) code. And of course, out of date comments are not only unhelpful, they can be mis-leading, and lead to errors. But of course, we are all excellent of keeping the documentation up to date aren’t we? 

So my advice, ditch the comments, if you can’t read the code, leave it alone.

 

I have re-cut that code above without comments, and with better spacing. I am not saying it is perfect, or the best way, it is just a way that I find better. I have ditched all the comments, none gave me anything, and I think the code is now ready for debugging.

As an aside, I have a routine that strips comments from code, which I wrote so I copuld strip those forum postings where comments gets in the way.

 

Sub Copy_and_Rename_To_New_Folder()
    Dim objFSO As New Scripting.FileSystemObject, objFolder As Scripting.folder, PathExists As Boolean
    Dim objFile As Scripting.File, strSourceFolder As String, strDestFolder As String
    Dim x, Counter As Integer, Overwrite As String, strNewFileName As String
    Dim strName As String, strMid As String, strExt As String
    Dim sSavePath3 As String

    Application.ScreenUpdating = False
    Application.EnableEvents = False

    strSourceFolder = “C:\Test”
    On Error Resume Next
    x = GetAttr(strDestFolder) And 0
    If Err = 0 Then

        PathExists = True
        Overwrite = MsgBox(“The folder may contain duplicate files,” & vbNewLine & _
        “Do you wish to overwrite existing files with same name?”, vbYesNo, “Alert!”)
        If Overwrite <> vbYes Then Exit Sub
    End If
    On Error Goto ErrHandler

    Set objFSO = CreateObject(“Scripting.FileSystemObject”)
    Set objFolder = objFSO.GetFolder(strSourceFolder)

    Counter = 0
    If Not objFolder.Files.Count > 0 Then Goto NoFiles

    For Each objFile In objFolder.Files

        strName = Left(objFile.Name, Len(objFile.Name) – 4
        strExt = Right(objFile.Name, 4)
        Dim strDate As String
        strNewFileName = “09 lqd ” & strName & ” TRS” & strExt
        objFile.Name = strNewFileName
        Counter = Counter + 1
    Next objFile

    Set objFile = Nothing: Set objFSO = Nothing: Set objFolder = Nothing

    Exit Sub

NoFiles:
    MsgBox “There Are no files or documents in : ” & vbNewLine & vbNewLine & _
    strSourceFolder & vbNewLine & vbNewLine & “Please verify the path!”, , “Alert: No Files Found!”

    Set objFile = Nothing: Set objFSO = Nothing: Set objFolder = Nothing

    Application.ScreenUpdating = True
    Application.EnableEvents = True

    Exit Sub ‘exit sub here to avoid subsequent actions

ErrHandler:
    MsgBox “Error: ” & Err.Number & Err.Description & vbCrLf & vbCrLf & vbCrLf & _
    “Please verify that all files in the folder are not currently open,” & _
    “and the source directory is available”
    Err.Clear ‘clear the error

    Set objFile = Nothing: Set objFSO = Nothing: Set objFolder = Nothing

    Application.ScreenUpdating = True
    Application.EnableEvents = True
End Sub

Sub FolderExists()
    Dim FSO
    Dim folder As String

    folder = “G:\Marketing\Market Price Guides\1Valeo Power Summaries”
    Set FSO = CreateObject(“Scripting.FileSystemObject”)

    If FSO.FolderExists(folder) Then

        MsgBox folder & ” is a valid folder/path.”, vbInformation, “Path Exists”
    Else

        MsgBox folder & ” is NOT a valid folder/path. “, vbInformation, ” Invalid Path”
    End If
End Sub


10 thoughts on “Keep It Simple”

  1. Bob,
    One of the best commenting guidelines I’ve seen is:
    Say Why the code does what it does not what it does.
    I just spent 1/2 hour trying to figure out why I had
    offset a range in some 4 year old code.

    Jim Cone

  2. Read a good programming book, so one dealing with pretty much any language other than BASIC, for examples of good commenting.

    OK, maybe a little too broad. If you can stand PL/I and FORTRAN, Kernighan & Plaugher’s ‘Elements of Programming Style’ is still a very, very good text. One of its main points is that clear code doesn’t require many comments.

    FWIW, I usually provide ISBN and page number or urls to sources for numeric algorithms I use. I don’t mind a lot of verbiage describing which exceptions are trapped and which error values are returned or state variables set.

  3. @Jim, I agree that if you add comments then that is what they should do. But in your example, when you wrote ithat line it probably seemed obvious at the time, you might just as well have not seen the rationale for a comment there. The only recourse then is to comment EVERY line of code, in which case you get int the comment hell I show above, and it takes even longer to make changes/debug.

    @Harlan, I have read many, non Basic, targetted books, and I understand the principles and how to comment well, I just think that it is rarely propely applied, and when it is, the value is debatable. I just find the code is easier to follow sans-comment. I will try t get that book, it seems to align with my philosophy. I also think that when a specific algorithm is used, a reference to such is a good idea, why re-invent that particlar wheel, and it wouldn’t over-comment in that case.

  4. Looks like you missed this bit of code:

    Dim objFSO As New Scripting.FileSystemObject

    .. then further down:

    Set objFSO = CreateObject(“Scripting.FileSystemObject”) ‘creates a new File System Object reference

  5. Bob I’m sure there are more than 2 of us!
    Before I scrolled down I thought the bad was going to be the name, and the obvious fact that it does far too many separable things.
    Jim, For the offset if you give the number a name (ie make it a const not a literal ‘magic number’) then it should be easy to maintain and understand.
    Harlan I don’t think most of the VBA we work with is that indepth. a lot of it is fairly trivial glue code and genuine ‘macro’ stuff. Deep complex algorithms I agree can benefit from some commenting, but not as intrusive as this junk Bob has found for our amusement. I love counter = 0 ‘set the counter to zero – class!

  6. Simon,

    Yes we can laugh, but I am serious. I really believe that commenting is over-egged, most comments I see are rubbish, most comments I write are rubbish, and as I say, they at minimum interrupt code readability, at worst can absolutely destroy the code readability (as above).

    The standards police have this as one of their pet crusades. A Word MVP recently posted a survey on the value of comments, and I saw absolutly no serious discussion, never mind dissent. I thought about it, but could not be bothered to argue with the world.

  7. I’m also with you on this Bob. I rarely use code. I hope that my code is well structured and easy to follow. I expect that anybody that wants to understand my code, or change it, had better know VBA! And where they don’t understand an element there is always the internet or a good book! I only ever use comments for me!

Leave a Reply

Your email address will not be published. Required fields are marked *

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>