How to make this macro safer?

[Alex20850]

[FONT=&quot]0
Low Priority ?

0 Views
Last Modified: 2018-06-29
Edit Question
[/FONT]
[FONT=&quot]I have used this Excel 2016 macro - that was written for me - but I have been told on another site that some parts of it are unsafe.
I am not familiar with VBA so I would like to make it safe to use.

"You shouldn't use dir() function to list files in a folder as it is an unsafe function.
It is better to use the FSO library.
The global ActiveSheet object, Sheets and Worksheets collections for the very same reason"


Sub InsertImages()
Dim WS As Worksheet
Dim StrFile As String
Dim strFolder As String
Dim strFilter As String

strFolder = ""
strFilter = "*.*"

With Application.FileDialog(mso<wbr style="font-size: 16px; font-family: inherit;">FileDialog<wbr style="font-size: 16px; font-family: inherit;">FolderPick<wbr style="font-size: 16px; font-family: inherit;">er)
.Title = "Choose folder"
.InitialFileName = "d:"
If .Show = -1 Then strFolder = .SelectedItems(1)
End With
StrFile = Dir(strFolder & "" & strFilter)
Do While Len(StrFile) > 0
Set WS = Sheets.Add(After:=Sheets(W<wbr style="font-size: 16px; font-family: inherit;">orksheets.<wbr style="font-size: 16px; font-family: inherit;">Count))
WS.Activate
ActiveSheet.Pictures.Inser<wbr style="font-size: 16px; font-family: inherit;">t(strFolde<wbr style="font-size: 16px; font-family: inherit;">r & "" & StrFile).Select
StrFile = Dir
Loop
End Sub[/FONT]
[FONT=&quot]
[/FONT]

 

[footoo]

"You shouldn't use dir() function to list files in a folder as it is an unsafe function.
It is better to use the FSO library.
The global ActiveSheet object, Sheets and Worksheets collections for the very same reason"

Unsafe in what way? Who says so?

https://msdn.microsoft.com/en-us/vba/language-reference-vba/articles/dir-function

https://support.microsoft.com/en-us/help/186118/how-to-use-filesystemobject-with-visual-basic

 

[Fluff]

Cross posted https://www.experts-exchange.com/questions/29106861/How-to-make-this-macro-safer.html#comments

While we do not prohibit Cross-Posting on this site, we do ask that you please mention you are doing so and provide links in each of the threads pointing to the other thread (see rule 13 here along with the explanation: Forum Rules).
This way, other members can see what has already been done in regards to a question, and do not waste time working on a question that may already be answered.

 

[Alex20850]

[FONT=&quot]The dir() function is unsafe, for the simple reason that it hold its criteria in an (unreachable) global scope that can be changed anytime by anyone. The Scripting.FileSystem Library offer a better alternative.

The global Sheets collection is unsafe, because it is implicitly refering to ActiveWorkbook.Sheets, and you have no guarantees about what the active workbook is (a simple user interraction is enough to change it).
Same goes with ActiveSheet (implicitly ActiveWorkbook.ActiveSheet<wbr style="font-size: 16px; font-family: inherit;">).

Additionaly, hungarian notation is a relic of the post, providing nothing usefull.
No need to select or activate anything while your function is executing, as it slow down the process, and provide an hugly user experience.

Finally, I'm not a big fan of pre-declaring variables (for lifetime reasons).

With all that taken into consideration, your updated code can look Something like the following:<code class="code-10 nolinks" id="code-20-42608681-1" style="font-variant-numeric: normal; font-variant-east-asian: normal; font-stretch: normal; font-size: 1.4rem; line-height: 17.5px; font-family: Courier, monospace; margin: 0px 3px; padding: 0px; color: black; display: block; counter-reset: code-line-number 0; overflow: auto; max-height: 50rem;">Public Sub InsertImages()
Dim fso As Object '// Scripting.FileSystemObject
Set fso = CreateObject("Scripting.FileSystemObject")

Dim folder As Object '// Scripting.folder
With Application.FileDialog(msoFileDialogFolderPicker)
.Title = "Choose folder"
.InitialFileName = "d:"
If .Show = -1 Then
Set folder = fso.GetFolder(.SelectedItems(1))
Else
Exit Sub
End If
End With

Dim wb As Excel.Workbook
Set wb = ThisWorkbook

With wb
Dim file As Object '//Scripting.file
For Each file In folder.Files
Dim ws As Excel.Worksheet
Set ws = .Sheets.Add(After:=.Sheets(.Worksheets.Count))

ws.Pictures.Insert file.Path
Next
End With
'// eventually, select or activate something here.
End Sub
</code>

<button name="codeSnippetId" type="button" value="code-20-42608681-1" class="button select-all" style="font-size: 1.2rem; font-family: inherit; padding: 0px; background-position: left top; background-repeat: no-repeat; border-width: 0px; border-style: initial; border-color: initial; cursor: pointer; line-height: 23px; overflow: visible; position: relative; vertical-align: middle; transition: background-color 0.2s ease-in-out; margin: 0px 0px 0px 8px; height: initial;">

</button>​

<label for="answerOrCommentView-memberFeedback-a42608681-select-button" style="font-size: 1.6rem; font-family: inherit; display: inline-block; height: 28px; line-height: 28px; vertical-align: top; color: rgb(0, 163, 224); margin-right: 0.5em;">Was this helpful?</label> <select name="memberFeedbackId" size="1" id="answerOrCommentView-memberFeedback-a42608681-select" style="font-size: 12px; font-family: inherit; font-weight: inherit; display: none;"> <option disabled="disabled" selected="selected" style="font-family: inherit;"></option> <option value="10" style="font-family: inherit;">Helpful</option> <option value="20" style="font-family: inherit;">Unhelpful</option> </select>





[/FONT]

 

[footoo]

There is a syntax error in the macro per your first post :

[COLOR=#333333]Sub InsertImages()
Dim WS As Worksheet
Dim StrFile As String
Dim strFolder As String
Dim strFilter As String

strFolder = ""
strFilter = "*.*"

With Application.FileDialog(mso<wbr style="font-size: 16px; font-family: inherit;">FileDialog<wbr style="font-size: 16px; font-family: inherit;">FolderPick<wbr style="font-size: 16px; font-family: inherit;">er)
.Title = "Choose folder"
.InitialFileName = "d:"
If .Show = -1 Then strFolder = .SelectedItems(1)
End With
StrFile = Dir(strFolder &[/COLOR][COLOR=#ff0000] "/" [/COLOR][COLOR=#333333]& strFilter)
Do While Len(StrFile) > 0
Set WS = Sheets.Add(After:=Sheets(W<wbr style="font-size: 16px; font-family: inherit;">orksheets.<wbr style="font-size: 16px; font-family: inherit;">Count))
WS.Activate
ActiveSheet.Pictures.Inser<wbr style="font-size: 16px; font-family: inherit;">t(strFolde<wbr style="font-size: 16px; font-family: inherit;">r & "" & StrFile).Select
StrFile = Dir
Loop
End Sub[/COLOR]

I cannot share the concerns about Dir and ActiveSheet being unsafe.
Is it not just a matter of using appropriate code when using these?

Any run time difference between Dir and Scripting.FileSystem Library?
Isn't Dir faster?

Re hungarian notation, it makes no difference one way or the other.
Personal preference applies.

Re "Finally, I'm not a big fan of pre-declaring variables (for lifetime reasons)".
This is merely personal preference. I prefer all variables to be pre-declared at the top.
(I wonder what is meant by "for lifetime reasons")

 

[footoo]

The following is a copy of a post on https://www.mrexcel.com/forum/redir...61/How-to-make-this-macro-safer.html#comments to illustrate how Dir is “unsafe :
-----------------------------------------------------------------------------------------------------------------
As a sample, consider the following code:

[COLOR=#0000cd]Public Sub listFile()[/COLOR]
[COLOR=#0000cd]    Const folderPath As String = "D:\Documents\Cmlvm"[/COLOR]
[COLOR=#0000cd]    Const filter As String = "*.*"[/COLOR]

[COLOR=#0000cd]    Dim filePath As String[/COLOR]
[COLOR=#0000cd]    filePath = Dir(folderPath & "\" & filter)[/COLOR]
[COLOR=#0000cd]    Do While (Len(filePath) > 0)[/COLOR]
[COLOR=#0000cd]        If fileExist(folderPath & "\" & filter) Then[/COLOR]
[COLOR=#0000cd]            Debug.Print filePath[/COLOR]
[COLOR=#0000cd]        End If[/COLOR]
[COLOR=#0000cd]        filePath = Dir[/COLOR]
[COLOR=#0000cd]    Loop[/COLOR]
[COLOR=#0000cd]End Sub[/COLOR]

Knowing that the directory (myFolder) is holding many files, yet the function only print one.
The reason lies wihin the fileExist function wich also use the dir function, and thus mess up with the search criterias:

[COLOR=#0000cd]Public Function fileExist(ByVal path As String) As Boolean[/COLOR]
[COLOR=#0000cd]    If Len(Dir(path)) > 0 Then[/COLOR]
[COLOR=#0000cd]        fileExist = True[/COLOR]
[COLOR=#0000cd]    Else[/COLOR]
[COLOR=#0000cd]        fileExist = False[/COLOR]
[COLOR=#0000cd]    End If[/COLOR]
[COLOR=#0000cd]End Function[/COLOR]

A function do not care on how called functions perform their job (and it should not), all it is interrested in is the service done.
Also, a function do not care on what the calling function do, and should not interfer.
Ehence why thje dir() function is unsafe.

-----------------------------------------------------------------------------------------------------------------

If appropriate coding is used, there is not a problem.
The Function should be :

Public Function fileExist(ByVal path As String) As Boolean
    If Len([COLOR=#ff0000]path[/COLOR]) > 0 Then
        fileExist = True
    Else
        fileExist = False
    End If
End Function

Or the Function can be eliminated altogether by amending the macro :

Public Sub xlistFile()
    Const folderPath As String = "D:\Documents\Cmlvm"
    Const filter As String = "*.*"
    
    Dim filePath As String
    filePath = Dir(folderPath & "\" & filter)
[COLOR=#ff0000]    Do While (Len(filePath) > 0)[/COLOR]
[COLOR=#ff0000]        Debug.Print filePath[/COLOR]
[COLOR=#ff0000]        filePath = Dir[/COLOR]
[COLOR=#ff0000]    Loop[/COLOR]
End Sub