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 tgo 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..
'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
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
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