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
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
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.
@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.
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
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!
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.
Bob
is not the world, just wannbe developers. Most of the agile folks think they are pants too.
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!
<< I rarely use code >> Comments, I mean comments!!! I’m still getting over anaesthetic, brain not with it!
Glad to hear that Jon. #3 and counting.