Skip to main content
Code Review

Return to Answer

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

Other than the hard-coded error numbers mentioned in @Snowbody's answer @Snowbody's answer...

Other than the hard-coded error numbers mentioned in @Snowbody's answer...

Other than the hard-coded error numbers mentioned in @Snowbody's answer...

Source Link
Mathieu Guindon
  • 75.5k
  • 18
  • 194
  • 467

There are a number of (minor) issues here.

#Re-raising errors

This is rather ugly:

Err.Raise Err.Number, Err.source, Err.Description, Err.HelpFile, Err.HelpContext

Per MSDN, Err.Raise can "rethrow" an error much cleaner than that:

All of the Raise arguments except Number are optional. If you omit optional arguments, and the property settings of the Err object contain values that have not been cleared, those values serve as the values for your error.

This means the above can be shortened to, Err.Raise Err.Number.


#Magic Constants

Other than the hard-coded error numbers mentioned in @Snowbody's answer...

Private Sub UserForm_QueryClose(Cancel As Integer, CloseMode As Integer)
 If CloseMode = 0 Then
 Cancel = True
 RaiseEvent Cancelled
 End If
End Sub

...0 is a magic value in CloseMode = 0; the VBA standard library defines the vbQueryClose enum for that purpose - replacing value 0 with vbQueryClose.vbFormControlMenu makes it much clearer that the condition is evaluating whether the CloseMode is related to the user clicking the red "X" in the form's control box.


#Cancellation

This is totally unacceptable UX - this error will bring up a VBA debugger window prompting the user to End execution or Debug the code... which makes it a feature that feels like a bug!

Private Sub view_Cancelled()
 'this error isn't trappable, but not raising it wouldn't cancel anything:
 Err.Raise ProgressIndicatorError.Error_OperationCancelled, TypeName(Me), ERR_OPERATION_CANCELLED
End Sub

Raising an untrappable error doesn't cancel the task in progress, it nukes it.

nuke

You have already cancelled the form's closing; all you need to do is to forbid the cancellation of the task in progress... unless the client code has an explicit way of handling cancellation - and you can let the client code know the user intends to cancel the task, by raising an event.

I would add a canCancel member to this, which would only be True when the "DoWork" code is located in a class module (i.e. when the ProgressIndicator instance can be a Private WithEvents field) - then you can leave it up to the client code to decide whether they want to nuke the long-running process, or handle it cleanly.

So you add a BeforeCancel event to the ProgressIndicator:

Public Event BeforeCancel(ByRef throw As Boolean)

And you raise it before the nuke goes off, to allow the client code to set throw to False and deal with cleanly cancelling the task:

Private Sub view_Cancelled()
 
 If Not this.canCancel Then Exit Sub
 
 Dim throw As Boolean
 throw = True
 RaiseEvent BeforeCancel(throw)
 
 'this error isn't trappable, but not raising it wouldn't cancel anything:
 If throw Then OnCancelledError
 
End Sub

Then the client code can have a Boolean flag to capture the cancelling state of the progress indicator:

Private WithEvents indicator As ProgressIndicator
Private isCancelling As Boolean

And deal with the BeforeCancel event like this:

Private Sub indicator_BeforeCancel(throw As Boolean)
 isCancelling = True
 throw = False
End Sub
Private Sub OnProgressCancelled()
 Err.Raise ProgressIndicatorError.Error_OperationCancelled, TypeName(Me), "Operation was cancelled."
End Sub

And now the "DoWork" code can periodically evaluate the isCancelling flag, and act accordingly:

For Each record In data
 
 If isCancelling Then OnProgressCancelled
 ...

...resulting in a clean cancellation:

operation was cancelled. 0/6 transactions committed.

lang-vb

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