Can this be done more efficiently?

The code does the following:

  1. Do Stuff
  2. Show UserForm and ask user to confirm the shown data is ok
  3. User can manipulate and controll excel as usual
  4. As soon as user clicks the "yes" button "do some other stuff" is called

The Module

Sub ControlDataUI()
Dim Ui As New UserForm1
Dim IsConfirmed As Boolean

Debug.Print "dostuff"

With Ui
    .Show (0)
    While Not .IsHiden
    If .IsCancelled Then Exit Sub
    IsConfirmed = .Confirmed
End With

Debug.Print "Do some more stuff!"

If IsConfirmed Then
    Debug.Print "SaveStuff"
End If

Debug.Print "I will die!!"
End Sub

The UserForm

Private Type TView
    IsCancelled As Boolean
    Confirmed As Boolean
    IsHiden As Boolean
End Type

Private this As TView
Public Property Get IsCancelled() As Boolean
    IsCancelled = this.IsCancelled
End Property
Public Property Get Confirmed() As Boolean
    Confirmed = this.Confirmed
End Property
Public Property Get IsHiden() As Boolean
    IsHiden = this.IsHiden
End Property

Private Sub CommandButton1_Click()
Debug.Print "YES!!!!"
this.Confirmed = True
this.IsHiden = True
End Sub

Private Sub CommandButton2_Click()
Debug.Print "NO?!?!"
this.Confirmed = False
this.IsHiden = True
End Sub

Private Sub UserForm_Click()

End Sub

Private Sub UserForm_QueryClose(Cancel As Integer, CloseMode As Integer)
this.IsCancelled = True
End Sub

Private Sub UserForm_Terminate()
Debug.Print "Murder! I was killed!"
End Sub

So the idea was in the right direction but the while loop had to contain a DoEvents in order to process the events comming from the user.


1. Modeless Userform

2. While loop with DoEvents until user confirmes

This allows for the user controll of the sheet and a confirmation at any time. The code will run after the user confirms the data is ok.


Yup, it's wrong1 :)

Forms (and literally everything in Windows) already run their own message loop: whenever you feel the need to wrap a form with a no-op "wait until user does something" DoEvents loop, you are essentially forcing your form into a procedural dialog paradigm, keeping the VBA runtime much, much more busy than it needs to be.

Embrace the objects!

The idiomatic way to handle this in VBA is to adopt an event-driven paradigm: instead of wait-looping for a state change and essentially "poll" your form periodically until its .IsHiden (typo?) state changes, have a presenter class that's responsible for dealing with the state of the form, by responding to its events.

Dim Ui As New UserForm1

Instead of a local (auto-instantiated?) object variable, make that a WithEvents variable at module level (that's why you need a class: only class modules can have WithEvents variables).

Option Explicit
Private WithEvents UI As UserForm1

Private Sub Class_Initialize()
    Set UI = New UserForm1
End Sub

Now this class can handle any of the UI events - whether they're inherited from the UserForm base class, or custom-defined on the UserForm1 default interface, like this:

Public Event ByeBye()

Private Sub CommandButton1_Click()
    Debug.Print "YES!!!!"
    this.Confirmed = True
    this.IsHiden = True
    RaiseEvent ByeBye
End Sub

Note the indentation is made more consistent (and arguably better, IMO) by adding an indent level for all members.

Doing this allows the presenter class (the one that declares a WithEvents instance of the form) to handle this ByeBye event:

Private Sub UI_ByeBye()
    'form is no longer displayed
End Sub

Note that this is an entirely different paradigm: you can't do stuff in the same procedure that's responsible for showing the form anymore - it forces you to separate the responsibilies!

1 Only because the form isn't modal though: a modal form suspends execution, which makes the explicit loop useless; the form's state can be read on the statement that immediately follows the call to .Show, since that next instruction will only run after the form is dismissed. That isn't the case for a non-modal form, so instead of loop-waiting, we need to embrace the asynchronous nature of the non-modal messenging, with events.

  • \$\begingroup\$ Not that it's particularly relevant to your review, but what do you mean by the line "whether they're inherited from the UserForm base class, or custom-defined on the UserForm1 default interface"? I was under the impression that the UserForm1 class defines an interface which extends the base UserForm class' interface (by inheriting methods and events like show etc. and defining new ones). But what's 'default' got to do with anything, we're not referring to the pre declared instance of UserForm1 right? Sorry, I'm not hugely familiar with these concepts since they can't be copied in VBA \$\endgroup\$ – Greedo Jan 28 at 17:19
  • \$\begingroup\$ @Greedo the "default interface" of a class is the interface presented by an object declared As [ClassName] - in other words As UserForm1 includes the members of UserForm inherited from the base class, and the members of UserForm1 defined in UserForm1, but not the members of IDialogView defined in some hypothetical IDialogView class that UserForm1 would be implementing with Implements IDialogView. \$\endgroup\$ – Mathieu Guindon Jan 28 at 17:23
  • \$\begingroup\$ @TinMan that sample code involves a modal form though. Non-modal forms demand a different paradigm, an event-driven approach. I don't recall writing about non-modal forms, since I don't use them much. But yeah, I'm definitely open for debate - who's the author? ;-) \$\endgroup\$ – Mathieu Guindon Jan 28 at 19:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.