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