Variable Naming
###Variable Naming
Standard VBA naming conventions have camelCase
for local variables and PascalCase
for other variables and names.
###Error handling
Error handling
###Extra
Extra
###Variable Naming
Standard VBA naming conventions have camelCase
for local variables and PascalCase
for other variables and names.
###Error handling
###Extra
Variable Naming
Standard VBA naming conventions have camelCase
for local variables and PascalCase
for other variables and names.
Error handling
Extra
In general, a For Each
loop is slower than a For Next
loop. So here -
If WS.Shapes.Count > 0 Then
For Each Shp In WS.Shapes
You could just do this:
For sheetindex = 1 To Source.Worksheets.Count
numberofshapes = Source.Sheets(sheetindex).Shapes.Count
If numberofshapes > 0 Then
For shapeindex = 1 To numberofshapes
Or better yet:
For sheetindex = 1 To Source.Worksheets.Count
Set targetSheet = Source.Sheets(sheetindex)
numberofshapes = targetSheet.Shapes.Count
If numberofshapes > 0 Then
For shapeindex = 1 To numberofshapes
Set targetShape = targetSheet.Shapes(shapeindex)
And target your shapes like targetShape.Name
.
Or you could wrap some of that in a With
clause, if you'd like.
In general, a For Each
loop is slower than a For Next
loop. So here -
If WS.Shapes.Count > 0 Then
For Each Shp In WS.Shapes
You could just do this:
For sheetindex = 1 To Source.Worksheets.Count
numberofshapes = Source.Sheets(sheetindex).Shapes.Count
If numberofshapes > 0 Then
For shapeindex = 1 To numberofshapes
Or better yet:
For sheetindex = 1 To Source.Worksheets.Count
Set targetSheet = Source.Sheets(sheetindex)
numberofshapes = targetSheet.Shapes.Count
If numberofshapes > 0 Then
For shapeindex = 1 To numberofshapes
Set targetShape = targetSheet.Shapes(shapeindex)
And target your shapes like targetShape.Name
.
Or you could wrap some of that in a With
clause, if you'd like.
Variable names - give your variables meaningful names.
Dim WS As Variant
Dim Shp As Shape
Dim Row As Long, Col As Long
Dim Response As Long
Why is WS
a Variant? I'd avoid using Row
as it's a default member. Also is Response
a Long
or is it a VbMsgBoxResult
type?
Dim targetSheet As Worksheet
Dim targetShape As Shape
Dim targetRow As Long
Dim targetColumn As Long
Dim confirmOverwrite As VbMsgBoxResult
Private Sub ListMacrosCalled(Optional ActSheet As Worksheet)
If you can, you should pass arguments ByVal instead of ByRef - which is standard. Also, usually if you have an optional argument, you can specify a default:
Private Sub ListMacrosCalled(Optional ByVal ActSheet As Worksheet = Sheet1)
That way this whole thing can be avoided:
If ActSheet Is Nothing Then
Set Source = ActiveWorkbook.Worksheets
Else
Source = Array(ActSheet)
End If
But, since your default is probably ActiveSheet
and you can't use that as default, you should make your argument Required instead of Optional.
Variable names - give your variables meaningful names.
Dim WS As Variant
Dim Shp As Shape
Dim Row As Long, Col As Long
Dim Response As Long
Why is WS
a Variant? I'd avoid using Row
as it's a default member. Also is Response
a Long
or is it a VbMsgBoxResult
type?
Dim targetSheet As Worksheet
Dim targetShape As Shape
Dim targetRow As Long
Dim targetColumn As Long
Dim confirmOverwrite As VbMsgBoxResult
Private Sub ListMacrosCalled(Optional ActSheet As Worksheet)
If you can, you should pass arguments ByVal instead of ByRef - which is standard. Also, usually if you have an optional argument, you can specify a default:
Private Sub ListMacrosCalled(Optional ByVal ActSheet As Worksheet = Sheet1)
That way this whole thing can be avoided:
If ActSheet Is Nothing Then
Set Source = ActiveWorkbook.Worksheets
Else
Source = Array(ActSheet)
End If
But, since your default is probably ActiveSheet
and you can't use that as default, you should make your argument Required instead of Optional.