CODEWITHSUNDEEP
vbaexceluserform
Svatik
Svatik

Reputation: 31

Userform makes Excel crash depending on condition

I'm facing a funny issue I don't understand.

In VBA, I want to load a Userform when the user double click on a cell, depending on the cell content. But depending on the condition I ask, Excel will crash once I exit the Sub (it keeps executing code even after the UserForm is closed).

This works:

Private Sub Workbook_SheetBeforeDoubleClick(ByVal Sh As Object, ByVal Target As Range, Cancel As Boolean)
    Dim myForm As New UserForm1
    If Target.Cells(1).Value = "" Then
        myForm.Show
    End If
    Debug.Print ("OK")
End Sub

And these three make Excel crash:

1.

Private Sub Workbook_SheetBeforeDoubleClick(ByVal Sh As Object, ByVal Target As Range, Cancel As Boolean)
    Dim myForm As New UserForm1
    If Target.Cells(1).Value <> "" Then
        myForm.Show
    End If
    Debug.Print ("OK")
End Sub

2.

Private Sub Workbook_SheetBeforeDoubleClick(ByVal Sh As Object, ByVal Target As Range, Cancel As Boolean)
    Dim myForm As New UserForm1
    If Not Target.Cells(1).Value = "" Then
        myForm.Show
    End If
    Debug.Print ("OK")
End Sub

3.

Private Sub Workbook_SheetBeforeDoubleClick(ByVal Sh As Object, ByVal Target As Range, Cancel As Boolean)
    Dim myForm As New UserForm1
    If Target.Cells(1).Value = "Test" Then
        myForm.Show
    End If
    Debug.Print ("OK")
End Sub

I'm using an empty UserForm for testing

Upvotes: 3

Views: 137

Answers (2)

Mathieu Guindon
Mathieu Guindon

Reputation: 71177

Cancelling the cell edit mode entry seems a pretty good way to limit the number of things that can go wrong.

There's more though.

Dim myForm As New UserForm1

That's creating an auto-instantiated UserForm1 object in local scope. Since Dim statements aren't executable, that means regardless of whether you Show the form or not, the object will be created as soon as the procedure scope is entered.

You can easily avoid creating that object unconditionally, and even avoid the need to declare a variable for it, using a With block:

Private Sub Workbook_SheetBeforeDoubleClick(ByVal Sh As Object, ByVal Target As Range, Cancel As Boolean)
    If Target.Cells(1).Value = "Test" Then
        Cancel = True
        With New UserForm1
            .Show
        End With
    End If
    Debug.Print "OK"
End Sub

That way the form instance only ever gets created if it needs to. Big huge kudos for avoiding the default instance trap though - you can read more about that on my blog.

Notice I also removed the extraneous parentheses around the Debug.Print argument list. Parentheses are actually actively harmful in procedure calls; you'll run into compile errors when you use them with multiple-parameter procedures, run-time errors when you need to pass an object reference, and logical bugs when you mean to pass a parameter ByRef - the parentheses used like this, are forcing VBA to evaluate the expression as a value, and always pass the result ByVal, regardless of whether the called procedure says it's taking a ByRef parameter.

Use parentheses when you're calling a function and capturing its return value in a local variable.

Upvotes: 1

tigeravatar
tigeravatar

Reputation: 26640

Try changing the code within your If block so that it does a Cancel = True to ignore the double-click action and also loads the form beforehand, like so:

If Target.Cells(1).Value = "Test" Then
    Cancel = True
    Load myForm
    myForm.Show
End If

Upvotes: 3

Related Questions