Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

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

added 472 characters in body
Source Link
Raystafarian
  • 7.3k
  • 1
  • 23
  • 60

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.

added 472 characters in body
Source Link
Raystafarian
  • 7.3k
  • 1
  • 23
  • 60

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.


added 472 characters in body
Source Link
Raystafarian
  • 7.3k
  • 1
  • 23
  • 60
Loading
Source Link
Raystafarian
  • 7.3k
  • 1
  • 23
  • 60
Loading
lang-vb

AltStyle によって変換されたページ (->オリジナル) /