CAD0808
CAD0808

Reputation: 11

can a loop solve and occurrence of excessive if statements?

I want to format a shape (Oval 1) using cell value "A1" I want to format the next shape (Oval 2) using cell value "A2" and so on.. I can get this done with repeated "If", "Else if" statements but I'm looking for a way to achieve this with a "for" loop or some other method to reduce the if/else statements and make the code simpler yet still effective.

Sub format_connector()
'
Application.ScreenUpdating = False

On Error Resume Next
ActiveSheet.Unprotect
ActiveSheet.Shapes.Range(Array("Oval 1")).Select

If Range("D5") = "GREEN" Then
    Call green
    ElseIf Range("D5") = "YELLOW" Then
        Call yellow
    ElseIf Range("D5") = "BLACK" Then
        Call black
    ElseIf Range("D5") = "BLACK/WHITE" Then
        Call black_white
    ElseIf Range("D5") = "RED" Then
        Call red
    ElseIf Range("D5") = "RED/WHITE" Then
        Call red_white
    ElseIf Range("D5") = "ORANGE" Then
        Call orange
    ElseIf Range("D5") = "ORANGE/WHITE" Then
        Call orange_white
    ElseIf Range("D5") = "BLUE" Then
        Call blue
    ElseIf Range("D5") = "BLUE/WHITE" Then
        Call blue_white
    ElseIf Range("D5") = "BROWN" Then
        Call brown
    ElseIf Range("D5") = "BROWN/WHITE" Then
        Call brown_white
    ElseIf Range("D5") = "VIOLET" Then
        Call violet
    ElseIf Range("D5") = "GRAY" Then
        Call gray
    ElseIf Range("D5") = "WHITE" Then
        Call white
    ElseIf Range("D5") = "WHITE/BLACK" Then
        Call white_black
    ElseIf Range("D5") = "WHITE/BLUE" Then
        Call white_blue
    ElseIf Range("D5") = "WHITE/BROWN" Then
        Call white_brown
    ElseIf Range("D5") = "408-4001-882" Then
        Call cavity_plug
    ElseIf Range("D5") = "408-4001-445" Then
        Call cavity_plug
    ElseIf Range("D5") = "408-4002-073" Then
        Call cavity_plug
    ElseIf Range("D5") = "408-4001-935" Then
        Call cavity_plug
    ElseIf Range("D5") = "BLANK" Then
        Call blank
End If

Upvotes: 1

Views: 55

Answers (3)

Variatus
Variatus

Reputation: 14383

How about this solution?

Sub format_connector()
    ' 008 26 Apr 2020

    Dim Ws As Worksheet
    Dim SubName As String
    Dim R As Long
    Dim i As Integer

    Set Ws = ActiveSheet                        ' Be safe! Call the sheet by name.
    On Error Resume Next
    Ws.Unprotect
    On Error GoTo 0

    Application.ScreenUpdating = False
    R = 5                                       ' start the loop at D5
    Do
        SubName = Replace(Ws.Cells(R, "D").Value, "/", "_")
        If SubName = "" Then Exit Do
        R = R + 1

        i = i + 1
        Ws.Shapes("Oval " & i).Select
        If Val(SubName) Then SubName = "cavity_plug"
        Application.Run SubName
    Loop
    Application.ScreenUpdating = True
End Sub

The above code was modified to select Ovals 1, 2 etc. for as many as there are parameters specified in column D, starting from D5.

Upvotes: 0

CAD0808
CAD0808

Reputation: 11

Sub FormatConnector()

Application.ScreenUpdating = False On Error Resume Next Call underscore 'converts to underscore between colors

Dim color As String

ActiveSheet.Shapes.Range(Array("Oval 1")).Select color = Range("D5").Value Call CAVITY_PLUG End If Application.Run color

ActiveSheet.Shapes.Range(Array("Oval 2")).Select

color = Range("D6").Value Application.Run color

ActiveSheet.Shapes.Range(Array("Oval 3")).Select

color = Range("D7").Value Application.Run color

Upvotes: 0

Mathieu Guindon
Mathieu Guindon

Reputation: 71187

Since every condition uses the same left operand in a comparison, the If...Else If...End If block could be expressed with a Select...Case...End Select block, and that would already reduce some of the repetition.

Private Function GetMacroName(ByVal source As Range) As String
    Select Case Range("D5")
        Case "YELLOW":
            GetMacroName = "yellow"
        Case "BLACK":
            GetMacroName = "black"
        Case "BLACK/WHITE"
            GetMacroName = "black_white"
        '...
        Case Else
            GetMacroName = "blank"
    End Select
End Function

And then you can use Application.Run to invoke the parameterless procedure:

Application.Run GetMacroName(Range("D5"))

You can use a loop to run this instruction for different ranges:

Dim sheet As Worksheet
Set sheet = ActiveSheet '<~ sure of that?

Dim i As Long
For i = 1 To N '<~ N=number of iterations; presumably the number of oval shapes
    Dim oval As Shape

    On Error Resume Next '<~ manually handle non-existing shape #i
    Set oval = sheet.Shapes("Oval " & i)
    On Error GoTo 0

    If Not oval Is Nothing Then
        Application.Run GetMacroName(sheet.Range("D" & 5 + i - 1)), oval
    End If

    Set oval = Nothing
Next

Alternatively we could iterate the sheet's Shapes collection:

Dim sheet As Worksheet
Set sheet = ActiveSheet '<~ sure of that?

Dim oval As Shape, i As Long
For Each oval In sheet.Shapes
    i = i + 1
    If Left(oval.Name, 4) = "Oval" Then
        Application.Run GetMacroName(sheet.Range("D" & 5 + i)), oval
    End If
Next

Note that in both cases, the macro receives the Shape object it works with. By passing parameters, you make your code less reliant on global state, and easier to follow: you shouldn't have to work out who's the caller 3 frames up the call stack that called .Select on some shape, to know what you're working with! Passing parameters makes things much simpler to debug later.

Public Sub Yellow(ByVal sh As Shape)
    sh.ForeColor = vbYellow
End Sub

Note that if that's all that's going on here, you should only have one macro instead:

Public Sub FormatOvalShape(ByVal oval As Shape, ByVal color As Long)
    oval.ForeColor = color
    '...
End Sub

..and let the caller parameterize the call - you'll reduce code duplication even further. Actually, it would remove the need to map macro names; instead we map color codes:

Dim sheet As Worksheet
Set sheet = ActiveSheet '<~ sure of that?

Dim oval As Shape, i As Long
For Each oval In sheet.Shapes
    If Left(oval.Name, 4) = "Oval" Then
        FormatOvalShape oval, GetColorCode(sheet.Range("D" & 5 + i))
    End If
    i = i + 1
Next

Upvotes: 3

Related Questions