Progress Bar Form

Question

Forms in MS Access are really just special glorified classes, so you can create new instances of them from code like this.

Dim someForm as Form_Form1
set someForm = New Form_Form1
someForm.visible = true

I decided to take advantage of this and create a progress bar form to display during long running processes. I created a form named ProgressBar with a rectangle and a text box.

• txtStatus
• boxProgress

Form ProgressBar

Option Compare Database
Option Explicit

' Class: Form_ProgressBar
' Popup progress bar

Private Const MaxBoxWidth As Long = 7200

Public Enum ePBarModeType
    PBarMode_Percent = 0
    PBarMode_Executing = 1
End Enum

Private mMode As ePBarModeType
Private mCurrentProgress As Long
Private mSteps As Long

Public Property Get PercentComplete() As Double
'read only
    PercentComplete = mCurrentProgress / mSteps * 100
End Property

Public Property Let Mode(PBarMode As ePBarModeType)
    mMode = PBarMode
End Property

Public Property Get Mode() As ePBarModeType
    Mode = mMode
End Property

Public Property Let CurrentProgress(lng As Long)
' Updating the CurrentProgress property updates the status of the Progress Bar

    mCurrentProgress = lng
    ' format #0 makes a 1 or 2 digit number without decimals
    If mMode = PBarMode_Percent Then
        Me.txtStatus = Format(Me.PercentComplete, "#0") & " % Complete"
    ElseIf mMode = PBarMode_Executing Then
        Me.txtStatus = "Executing..."
    End If
    ' boxProgress.Width = a percentage of maximum box width
    Me.boxProgress.Width = (mCurrentProgress / mSteps) * MaxBoxWidth
    Me.Repaint
    DoEvents
End Property

Public Property Get CurrentProgress() As Long
    CurrentProgress = mCurrentProgress
End Property

Property Let steps(lng As Long)
    mSteps = lng
End Property

Public Sub init(steps As Long, Mode As ePBarModeType, Optional strCaption As String = "Loading...")
    Me.Mode = Mode
    Me.Caption = strCaption
    mCurrentProgress = 0
    mSteps = steps
    Me.txtStatus = "Ready"
    Me.boxProgress.Width = 0
    Me.Visible = True
End Sub

Example Calls

Private Sub exampleCall1()
' example call for using progress bar with a looping process
    Dim pbar As Form_ProgressBar
    Dim i As Long
    Dim steps As Long
    steps = 100000

    Set pbar = New Form_ProgressBar
    With pbar
        .init steps, PBarMode_Percent, "Hey, I'm working here!"
        For i = 1 To steps
            ' do something in a loop

            ' update progress
            .CurrentProgress = i
        Next i
    End With
    Set pbar = Nothing
End Sub


Private Sub exampleCall2()
' example call for using progress bar with an adodb.command
    Dim bimic As New BiMic_Wrapper
    Dim cmd As New ADODB.Command
    Dim prm As ADODB.Parameter

   DoCmd.HourGlass True

    With cmd
        .ActiveConnection = bimic.Connection
        .CommandType = adCmdStoredProc
        .CommandTimeout = 0
        .CommandText = "dbo.uspSomeStoredProcedure"
    End With

    'must execute command async
    cmd.Execute , , adAsyncExecute

    Dim pbar As New Form_ProgressBar
    Dim i As Long
    pbar.init 10000, PBarMode_Executing, ""

    Do While cmd.STATE = adStateExecuting
        For i = 0 To 10000
            pbar.CurrentProgress = i
        Next i
    Loop
    Set pbar = Nothing

    DoCmd.Hourglass False

End Sub

Concerns

• Am I really gaining anything from using a full fledged property for Mode? It's a simple Get & Let. Would it be cleaner to simply use a Public variable instead?
• Am I doing enough to ensure that the status doesn't display anything stupid? (Like displaying 103% complete.)

• I feel like this is a little obscure. I was thinking of splitting the logic into a private function with a decent name. Is it worth it? What would you name it?

  ' boxProgress.Width = a percentage of maximum box width
  Me.boxProgress.Width = (mCurrentProgress / mSteps) * MaxBoxWidth
  

Answer 1

Would it be cleaner to simply use a Public variable instead?

You're not really asking that question. I'm not really telling you that a property is always going to be cleaner than exposing a public field. ...Well look at that, I just did!

Point is, yes, any property could also be a public field. Heck, any public field might as well be a global variable instead. See where I'm going? Encapsulation is awesome, embrace it ;)

feel like this is a little obscure. I was thinking of splitting the logic into a private function with a decent name. Is it worth it?

' boxProgress.Width = a percentage of maximum box width
  Me.boxProgress.Width = (mCurrentProgress / mSteps) * MaxBoxWidth

All by itself, I see nothing obscure about this line; I'm not even sure I'd have that explanatory comment. But if you zoom out a little, and include a bit of context here...

Public Property Let CurrentProgress(lng As Long)
' Updating the CurrentProgress property updates the status of the Progress Bar

    mCurrentProgress = lng
    ' format #0 makes a 1 or 2 digit number without decimals
    If mMode = PBarMode_Percent Then
        Me.txtStatus = Format(Me.PercentComplete, "#0") & " % Complete"
    ElseIf mMode = PBarMode_Executing Then
        Me.txtStatus = "Executing..."
    End If
    ' boxProgress.Width = a percentage of maximum box width
    Me.boxProgress.Width = (mCurrentProgress / mSteps) * MaxBoxWidth
    Me.Repaint
    DoEvents
End Property

It's a property setter! I think a setter's job ends here:

    mCurrentProgress = lng

The rest of what happens in that Let-setter belongs in another method, perhaps something like... Refresh?

Public Property Let CurrentProgress(ByVal value As Long)
' Updating the CurrentProgress property updates the status of the Progress Bar

    mCurrentProgress = value
    Refresh

End Property

I wouldn't stop at Refresh - you have an Enum type, but you're If-ElseIfing through it. Use a Select Case here, Mr. Maintainer will like you when he needs to add a PBarMode_ManualCaption execution mode that displays an ad-hoc progress message ("Reticulating splines...") at each step; if there's a dedicated UpdateProgressMessage public method, it's even easier.

Your current implementation will [re-]set the Executing... text at every update in progress; I would make that message mode only update the progress message once instead.

And lng isn't a very good name for a value that represents progress. I like having a convention that properties themselves define the meaning of their paramater value with their name, so all my property [let-]setters take a value parameter. Maybe I'm just being lazy with naming though. But lng looks like a disemvoweled long... which looks like a Hungarian prefix without an actual variable name, a bit like str for a String.

So to recap on the CurrentProgress property, I would:

• Rename the parameter to value, and pass it ByVal instead of implicitly ByRef.
• Extract an UpdateProgressMessage method, to determine the status text.
• Extract an UpdateProgressValue method, to determine the length/value of the progress bar.

• Extract an Update or Refresh method, to call the other two extracted method:

  Private Sub Refresh()
  
      UpdateProgressMessage
      UpdateProgressValue
  
      Me.Repaint
      DoEvents
  
  End Sub
  

---

I cringe whenever I see a variable, field or property that's just called Mode. It's not you, it's just.. past experience working with someone's code where everything had a Mode, and Mode could be just about anything. So today I'm left with a severe reluctance to call something Mode. Maybe Mode is fine, but I'd prefer MessageDisplayMode, or MessageMode, or ProgressMessageMode... anything.

---

I don't like the m and e prefixes. The enum is easy to fix; the private fields though...

Private mMode As ePBarModeType
Private mCurrentProgress As Long
Private mSteps As Long

I know you've more than once commented on why I'm doing this, but I'd recommend shoving them into a Private Type - that way you can call them the same thing as the property they're backing:

Private Type TProgressBarMembers
    Mode As ePBarModeType
    CurrentProgress As Long
    Steps As Long
End Type

...and only have a single private field:

Private this As TProgressBarMembers

---

I don't like underscores either. The class itself is called Form_ProgressBar... VBA uses the underscore to denote event handlers and interface implementations; using it in an identifier feels wrong.

I'd rename the enum to something like this:

Public Enum ProgressBarDisplayMode
    DisplayPercent
    DisplayExecuting 
End Enum

Note that you don't need to specify any values when the first member's value is 0.

---

Overall I like the idea, but I find the names could use some fine-tuning. Other than that, you only have a few minor tweaks to make that code easier to extend, well done!

Answer 2

Adding the final version of the code here for anyone interested.

Option Explicit

' ***************************************************************
' Class: Form_ProgressBar 
' Popup progress bar 
' 
' Author: Christopher J. McClellan 
'     http://christopherjmcclellan.wordpress.com/
' Significant input from Mat's Mug 
'     http://codereview.stackexchange.com/users/23788/mats-mug
'
' Published under Creative Commons Attribution-Share Alike 
' http://creativecommons.org/licenses/by-sa/3.0/ 
'
' You are free to change, distribute, and pretty much do 
'  whatever you like with the code, but you must give credit 
'  to the original author and publish any derivative of this 
'  code under the same license. 
' ***************************************************************

Public Enum ProgressBarDisplayMode
    PBarDisplayPercent
    PBarDisplayExecuting
End Enum

Private Type TProgressBarMembers
    Mode As ProgressBarDisplayMode
    CurrentProgress As Long
    Steps As Long
End Type

Private Const maxBoxWidth As Long = 7200
Private Const executingMessage As String = "Executing..."

Private this As TProgressBarMembers

Public Property Get PercentComplete() As Double
'read only
    PercentComplete = this.CurrentProgress / this.Steps * 100
End Property

Public Property Let Mode(newValue As ProgressBarDisplayMode)
    this.Mode = newValue
End Property

Public Property Get Mode() As ProgressBarDisplayMode
    Mode = this.Mode
End Property

Public Property Let CurrentProgress(newValue As Long)
    this.CurrentProgress = newValue
    ' keep the graphics in sync
    RepaintMe
End Property

Public Property Get CurrentProgress() As Long
    CurrentProgress = this.CurrentProgress
End Property

Property Let Steps(newValue As Long)
    this.Steps = newValue
End Property

Public Sub Init(Steps As Long, Mode As ProgressBarDisplayMode, Optional Caption As String = "Loading...")
    Me.Mode = Mode
    Me.Caption = Caption
    this.CurrentProgress = 0
    this.Steps = Steps
    Me.boxProgress.Width = 0

    Select Case Mode
        Case PBarDisplayExecuting
            Me.txtStatus = executingMessage
        Case Else
            Me.txtStatus = "Ready"
    End Select

    Me.Visible = True
End Sub

Private Sub RepaintMe()

    If Not this.Mode = PBarDisplayExecuting Then
        UpdateProgressMessage
    End If

    UpdateBoxWidth

    Me.Repaint
    DoEvents
End Sub

Private Sub UpdateProgressMessage()
    Select Case this.Mode
        Case PBarDisplayPercent
            ' format #0 makes a 1 or 2 digit number without decimals
            Me.txtStatus = Format(Me.PercentComplete, "#0") & " % Complete"
        Case PBarDisplayExecuting
            Me.txtStatus = executingMessage
    End Select
End Sub

Private Sub UpdateBoxWidth()
    Me.boxProgress.Width = (this.CurrentProgress / this.Steps) * maxBoxWidth
End Sub

Answer 3

Mat's Mug covered the issues with the form/class itself pretty well, but I'd like to mention a few things about exampleCall2(). This is boilerplate code that will be used anytime you call a long running ADODB.Command. It's absolutely worth extracting it into a method that takes in a command as an argument.

Private Sub ExecuteWithProgressBar(cmd as ADODB.Command)

    DoCmd.HourGlass True

    'must execute command async
    cmd.Execute , , adAsyncExecute

    Dim pbar As New Form_ProgressBar
    Dim i As Long
    pbar.init 10000, PBarMode_Executing, ""

    Do While cmd.STATE = adStateExecuting
        For i = 0 To 10000
            pbar.CurrentProgress = i
        Next i
    Loop
    Set pbar = Nothing

    DoCmd.Hourglass False

End Sub

Which still leaves a few issues to be dealt with.

1. You're messing with the UI, so you need to have an error handler that turns the hour glass mouse pointer back off. While you're at it, make sure to Set pbar = nothing to make sure it's removed from the screen.

2. This comment isn't very helpful.

   'must execute command async
   

   Of course we have to execute asynchronously! It's so obvious! Wait... Why do we have to execute async??? Try this instead.

   'must execute command async so 
   ' the code doesn't wait for cmd 
   ' to finish before showing pbar
   

3. Instead of hard coding the status message and number of steps to take, use some optional parameters with default values.

ExampleCall1 is boilerplate code too, but much harder to get rid of without delegates (which vba doesn't support). Any attempt to do so would probably cause more harm than good in my opinion.