CODEWITHSUNDEEP
excelvbaif-statement
aye cee
aye cee

Reputation: 193

How can I get this VBA IF THEN Statement to work

I have a multi-select ListBox for selecting "colours" and have copied the code into a second module and renaming as required, in order to create a second ListBox for "tags". There is also code in Worksheet SelectionChange that I am trying to amend in order to get both of them working.

This is the code for 1 ListBox

Private Sub Worksheet_SelectionChange(ByVal Target As Range)
    If Target.Cells.Count > 1 Then Exit Sub
    If Not Intersect(Target, ColourArea(ActiveSheet)) Is Nothing Then 
        If Target.Column = 8 And Target.Offset(0, -6).Value = "Variable" Then
            CreateColourPopUp Target
        End If
    Else 
        DeleteAllPopUps Target
End Sub

And below is the code with my attempt at adding in the 2nd ListBox. Everything I have tried alternates with "block if without end if" errors, to one of them working, to none. How can I fix this code?

Private Sub Workbook_SheetSelectionChange(ByVal Sh As Object, ByVal Target As Range)
    If Target.Cells.Count > 1 Then Exit Sub
    If Not Intersect(Target, ColourArea(ActiveSheet)) Is Nothing Then
        If Target.Column = 7 And Target.OFFSET(0, -5).Value = "variable" Then
            CreateTagPopUp Target
        End If
        If Not Intersect(Target, TagArea(ActiveSheet)) Is Nothing Then
            If Target.Column = 8 And Target.OFFSET(0, -6).Value = "variable" Then 
                CreateColourPopUp Target
            End If
        Else
            DeleteAllTagPopUps Target
            DeleteAllColourPopUps Target
        End If
    End If
End Sub

Upvotes: 0

Views: 139

Answers (2)

Variatus
Variatus

Reputation: 14383

Apart from the missing Then spotted by @Vincent G, your procedure sports faulty logic. Because the second test runs irrespective of the result of the first, the TagPopUp created as a result of the first test can be deleted as a result of the second.

If this is intentional it isn't good or sensible practice. Instead, your code appears as if it was intentionally made complicated. Look at the ranges ColourArea(ActiveSheet) and TagArea(ActiveSheet). The ActiveSheet in question doubtlessly is the object assigned to the Sh variable. It isn't good practice to refer to the same object by different names.

The two ranges are either overlapping or single-column. In either case the two Intersect tests should be nested, followed by a Select statement, like *If Target is within the range Then Select different processes for column 7, column 8 and Else".

Target.Offset(0, -6), relating to column 8, and Target.Offset(0, -5), relating to column 7 would appear to both specify Sh.Cells(Target.Row, 2). Why hide this fact so artfully? In fact, it is a key component of your procedure's logic and "7 - 5" isn't any less hard-coded than "2". By paying due respect to this fact you might end up with a procedure like the one below.

Private Sub Workbook_SheetSelectionChange(ByVal Sh As Object, ByVal Target As Range)

    With Target
        If .Cells.CountLarge = 1 Then
            If Not Intersect(Target, ColourArea(Sh)) Is Nothing Then
                If Sh.Cells(.Row, 2).Value = "variable" Then
                    Select Case .Column
                        Case 7
                            CreateTagPopUp Target
                        Case 8
                            CreateColourPopUp Target
                    End Select
                Else
                    DeleteAllTagPopUps Target
                    DeleteAllColourPopUps Target
                End If
            End If
        End If
    End With
End Sub

This procedure isn't tested. However, the intention isn't to provide code that you can copy, paste and use happily ever after. Rather, I want to show how to structure code to be transparent so as to avoid the traps built into the procedure you published that lead to your having to ask a question at all. The above one you can fix yourself. Please do so if it needs fixing.

Upvotes: 1

braX
braX

Reputation: 11755

You are missing and End If

Private Sub Worksheet_SelectionChange(ByVal Target As Range)
    If Target.Cells.Count > 1 Then Exit Sub
    If Not Intersect(Target, ColourArea(ActiveSheet)) Is Nothing Then 
        If Target.Column = 8 And Target.Offset(0, -6).Value = "Variable" Then
            CreateColourPopUp Target
        End If
    Else 
        DeleteAllPopUps Target
    End If ' <---- add an End If
End Sub

Upvotes: 0

Related Questions