4
\$\begingroup\$

I'm programming a tool in Excel which reads two exported .csv files, performs some calculations based on Dates and Strings and populates a dashboard to be copied into a Google Sheet daily report.

Although the exported data would be no more than 150 rows (across two worksheets), I wrote a small routine to toggle "FastMode".

FastMode is for updating the following Application object properties;

  • ScreenUpdating
  • Calculation
  • DisplayAlerts

As the need for the tool was very sudden (as the only option was manually count - I don't like tedious counting) the initial code was very poorly written and the FastMode toggle was just a block of code in the main Sub with procedure level declarations used to capture the 'current' values for these properties and a public level boolean for IsFastMode to evaluate if FastMode was toggled on or off.


I'm now revising the code, using the opportunity to practise and learn VBA programming skills on a 'real life' problem.

I've revised the FastMode part of the code a few times now, first to it's own Public Sub ToggleFastMode in the main routine Module with arguments to allow passing the procedure level variables for the 'current' values.

I've now replaced the Public IsFastMode boolean with a Class Module property Status which has also been updated as the DefaultMember and moved the ToggleFastMode Toggle sub into the new Class Module.

FastMode Class Module

'@Folder("DailyJobsUpdated")
'@PredeclaredId
Option Explicit

Private Type ToggleFastMode
    NewStatus As Boolean
End Type
Private This As ToggleFastMode

'@DefaultMember
Public Property Get Status() As Boolean
    Status = This.NewStatus
End Property

Public Property Let Status(ByVal NewValue As Boolean)
    This.NewStatus = NewValue
End Property

Public Sub Toggle(ByVal ScreenUpdate As Boolean, ByVal Calculate As Long, ByVal DisplayAlert As Boolean)
    With Application
        If FastMode Then
            .ScreenUpdating = ScreenUpdate
            .Calculation = Calculate
            .DisplayAlerts = DisplayAlert
            FastMode = False
        ElseIf Not FastMode Then
            .ScreenUpdating = False
            .Calculation = xlCalculationManual
            .DisplayAlerts = False
            FastMode = True
        End If
    End With
End Sub

This would be called from within the main routine like so:

...
    Dim ScreenUpdateBool As Boolean
    Dim CalculationMethodConst As Long
    Dim DisplayAlertBool As Boolean
    
    With Application
        ScreenUpdateBool = .ScreenUpdating
        CalculationMethodConst = .Calculation
        DisplayAlertBool = .DisplayAlerts
    End With
    
    On Error GoTo Safety
    
    If Not FastMode Then FastMode.Toggle ScreenUpdateBool, CalculationMethodConst, DisplayAlertBool
...

As I'm still learning and wrapping my head around using Class Modules in my code, I'm specifically interested in;

  • Is the implementation of the Toggle sub into the Class Module the appropriate location to house this routine?
  • Is setting the Status property as the default member (using RubberDuck) a "bad" idea? (I'm not too worried about every little possibility of issues, just if there are common problems run into by setting default members in Class Modules).
  • Have I broken any general rules with the way I've written this Class Module?
  • Will it be more correct to set additional properties in the Class to be used in place of the Procedure level variables that are passed to the Toggle sub?

Any other feedback is welcomed.

\$\endgroup\$
4
  • 3
    \$\begingroup\$ Sees '@Folder(). Smiles at another RubberDuck user! \$\endgroup\$
    – FreeMan
    Jun 15 '21 at 13:44
  • 2
    \$\begingroup\$ I would store the previous values in private properties. Then when it's turned off you restore the previous values, instead of hard coded values. You also need some error handling to avoid leaving it in an undesirable state. \$\endgroup\$
    – HackSlash
    Jun 15 '21 at 16:03
  • 1
    \$\begingroup\$ Well done on practising writing classes. Unfortunately, the use of a class in this particular case is completely the wrong solution. \$\endgroup\$
    – Freeflow
    Jun 16 '21 at 8:46
  • 1
    \$\begingroup\$ I've left an explanation on this topic a while ago here and explained why it's correct to store/restore the state without making any assumptions. Just the first section is of interest to you. \$\endgroup\$ Jun 18 '21 at 14:40
5
\$\begingroup\$

About "Fast" Code

I take [mild] issue with "fast mode" and similar "optimize speed" wordings, because code that needs these switches toggled is very often code that is simply inefficient, and turning off calculations and screen repaints makes the macro complete faster, without making it any more efficient. Minimizing worksheet accesses (both reading and writing) normally makes the code run much faster, and when a macro involves a single worksheet read and a single worksheet write operation, toggling calculations and repaints off isn't going to change anything.

That said, there are times when such toggling is needed and/or beneficial. I'd go with HostApplicationState or something similarly neutral as far as alluding to any performance gains goes - simply because when the code is efficient in the first place, then the effect of these toggles is very marginal.


Naming

FastMode aside, NewStatus is confusing and has no reason to not be just Status: one of the advantages of using a Private Type for the backing field of properties, is that you get to name each field exactly as per the property member it's backing. Naming it differently defeats that purpose and makes Property members more confusing than necessary.

The NewValue parameter name is appropriate for a Property Let (or Set) value parameter identifier name; Value or RHS (for right-hand-side) would also be appropriate.


State Corruption

The predeclared instance is stateful, which comes with a number of implications: the state is effectively global, and FastMode.Toggle is working off that global state... but the Status property being writable is making it very easy to misuse the class and essentially corrupt the state, which would be causing bugs downstream.

Because the Toggle method assumes an enabled initial state, nesting instances would also corrupt the state.

Dim Test1 As FastMode '<~ this declaration is always legal
Set Test1 = New FastMode '<~ local instance has its own Status

FastMode.Toggle True, True, True '<~ Boolean arguments are bad for readability
Debug.Print FastMode '<~ default instance Status is True
Debug.Print Test1 '<~ local instance Status is still False
Test1 = True '<~ local instance write to Status is legal
FastMode = False '<~ manual toggle doesn't actually toggle the Application state

The Boolean parameters are confusing and should be avoided altogether: it's hard to tell whether the Toggle method is expecting True or False: the method isn't actually "toggling" the state so "true" isn't saying "yeah I want to toggle this one" but rather "make it true/false as I say", which is leaking the encapsulated global state into the abstraction.

So the first thing I would change would be to make Status read-only by outright deleting the Property Let member. The property being the class' default member isn't ideal, because it is parameterless. With the default/predeclared instance, that makes the meaning of a FastMode token highly context-dependent. Parameterless default members are illegal in VB.NET where the Set keyword is deprecated. Now, the Status property is a Boolean data type, so resolving the default member call isn't too hard: the member implicitly invoked is defined in the FastMode class. When a class' default member returns another object type (which could have its own default member), resolving default member calls becomes a recursive operation, and we're looking at code that says one thing and does another.

This could be fixed in two ways:

  1. Query This.FastMode in the Toggle method, making it work off the state of the current instance, i.e. each instance of the class gets its own state.
  2. Return FastMode.Status rather than This.Status in the Property Get Status member; conditionally return This.Status when Me Is FastMode: only the default instance actually holds any state.

Going with 2 is simpler, but then it makes instances of the FastMode class rather deceptive, since the state is actually held in the default instance: because no instance is holding any state, it becomes very difficult to justify using a class over a standard module here, because OOP and global state don't really work well together.


Alternative

I would suggest a simpler API involving a With block to eliminate the temporal coupling that forces the client code to invoke the Toggle method - the calling code might look like this:

With FastMode.Toggle
   '...do stuff...
End With

Or like this:

With FastMode.Toggle(AppState_Calculation Or AppState_ScreenUpdating)
   '...do stuff...
End With

Side note: this Or operator is bitwise-OR; a + operator could be used instead, but using Or in a bitwise context emphasizes the bitwise nature of the enum values.

The class' Initialize handler would be responsible for encapsulating the initial application state at instance level, and the Terminate handler would be responsible for restoring it... except for the default instance.

Because each instance is storing the application state as it is at the moment of initialization, instances can be safely nested; when an instance goes out of scope, application state gets restored to what it was when that instance was created. Guarding the mechanism against a stateful default instance seems wise here.

The Toggle method becomes essentially a factory method that toggles the application state, optionally only affecting a subset. The result of this, is a more robust API that makes both With New FastMode and With FastMode.Toggle behave identically. Since a factory method is involved, we want to supply and implement an explicit interface that I'll call IAppState here.

That IAppState interface might look like this - note that all we need of this interface is for it to provide a way to access an instance's encapsulated initial state, so that's where we'll want to have a property per setting:

'@ModuleDescription "An object that encapsulates initial application state."
'@Interface
Option Explicit

Public Property Get InitialCalculation() As XlCalculation
End Property

Public Property Get InitialScreenUpdating() As Boolean
End Property

Public Property Get InitialDisplayAlerts() As Boolean
End Property

Next we'd have a HostApplicationState class that implements this interface:

'@ModuleDescription "An object that toggles global application state as needed."
'@PredeclaredId
Option Explicit
Implements IAppState

Public Enum AppStateEnum
    AppState_None = 0

    'underlying values must be powers of 2:
    AppState_Calculation = 1
    AppState_ScreenUpdating = 2
    AppState_DisplayAlerts = 4

    'convenience member to include all properties:
    AppState_All = AppState_Calculation Or _
                   AppState_ScreenUpdating Or _
                   AppState_DisplayAlerts
End Enum

Private Type TState
    AppProperties As AppStateEnum
    InitialCalculation As XlCalculation
    InitialScreenUpdating As Boolean
    InitialDisplayAlerts As Boolean
End Type

Private This As TState

'@Description "Disables the specified global application properties; properties will toggle back on when object goes out of scope."
Public Function Toggle(Optional ByVal AppProperties As AppStateEnum = AppState_All) As IAppState
    Dim Result As HostApplicationState
    Set Result = New HostApplicationState '<~ Initialize handler runs here
    Result.AppProperties = AppProperties

    If HasFlag(AppProperties, AppStateEnum.AppState_Calculation) Then
        Application.Calculation = xlCalculationManual
    End If

    If HasFlag(AppProperties, AppStateEnum.AppState_ScreenUpdating) Then
        Application.ScreenUpdating = False
    End If

    If HasFlag(AppProperties, AppStateEnum.AppState_DisplayAlerts) Then
        Application.DisplayAlerts = False
    End If

    Set Toggle = Result
End Function

'@Description "Gets or sets the properties managed by this instance."
Public Property Get AppProperties() As AppStateEnum
    AppProperties = This.AppProperties
End Property

Public Property Let AppProperties(ByVal Values As AppStateEnum)
    This.AppProperties = Values
End Property

'@Description "A helper function performing bitwise logic for flag enums."
Private Function HasFlag(ByVal value As Long, ByVal flag As Long) As Boolean
    HasFlag = (value And flag) = flag
End Function

Private Sub Class_Initialize()
    'Default instance is stateless:
    If Me Is HostApplicationState Then Exit Sub
    'affect all managed properties by default (use .Toggle to narrow it down):
    This.AppProperties = AppStateEnum.AppState_All

    This.InitialCalculation = Application.Calculation
    This.InitialScreenUpdating = Application.ScreenUpdating
    This.InitialDisplayAlerts = Application.DisplayAlerts
End Sub

Private Sub Class_Terminate()
    'Default instance is stateless:
    If Me Is HostApplicationState Then Exit Sub
    'only restore the properties that were toggled...
    If HasFlag(This.AppProperties, AppStateEnum.AppState_Calculation) Then
        Application.Calculation = This.InitialCalculation
    End If
    If HasFlag(This.AppProperties, AppStateEnum.AppState_ScreenUpdating) Then
        Application.ScreenUpdating = This.InitialScreenUpdating
    End If
    If HasFlag(This.AppProperties, AppStateEnum.AppState_DisplayAlerts) Then
        Application.DisplayAlerts = This.InitialDisplayAlerts
    End If
End Sub

Private Property Get IAppState_InitialCalculation() As XlCalculation
    IAppState_InitialCalculation = This.InitialCalculation
End Property

Private Property Get IAppState_InitialScreenUpdating() As Boolean
    IAppState_InitialScreenUpdating = This.InitialScreenUpdating
End Property

Private Property Get IAppState_InitialDisplayAlerts() As Boolean
    IAppState_InitialDisplayAlerts = This.InitialDisplayAlerts
End Property

Implemented like this, state is no longer global, and each instance encapsulates its own Initial[ApplicationProperty] values to use for restoring the Application state to exactly what it was before the object came into existence.

Now the API is looking like this:

With HostApplicationState.Toggle '<~ all managed properties get disabled
    'note: the only available members are the .Initial[ApplicationProperty] getters
    '...do stuff...
End With '<~ all managed properties get toggled back on

That is functionally equivalent to this - note the Toggle member call; including it in the Initialize handler would remove the need to have it in the client code here, but then it would also make it needlessly invoked twice when using the preferred factory method, and we don't want a side-effecting Initialize handler anyway so if client code decides to New up the class, Toggle needs to be explicitly invoked:

With New HostApplicationState
    .Toggle '<~ all managed properties get disabled
    'note: .Toggle and .AppProperties members are accessible, but not the .Initial[ApplicationProperty] getters.
    '...do stuff...
End With '<~ all managed properties get toggled back on

While it "works", it isn't leveraging the class' default instance, which makes the VB_PredeclaredId attribute useless. Still, it would probably work fine without breaking anything. Obviously it can be abused by changing the AppProperties value before the object goes out of scope, but then such wrong code would look wrong, and that's perfect (and guard clauses can actively prevent such misuse by throwing an error in the Property Let scope).

And if do stuff involves invoking a procedure that specifically needs alerts turned off, then that macro can do this:

With HostApplicationState.Toggle(AppState_DisplayAlerts)
    'note: Application.DisplayAlerts state before the With block is irrelevant
    '...do stuff
End With

And when that instance goes out of scope (i.e. when the With block is exited - note that this covers jumping out of the With block on error); if that instance is nested and the outer instance has already toggled DisplayAlerts, then this inner instance is redundant because its InitialDisplayAlerts is going to be False - by encapsulating the initial state, it's making fewer assumptions.

Perhaps Manage would be a better name than Toggle here:

With HostApplicationState.Manage(AppState_Calculation Or AppState_ScreenUpdating)
    '...do stuff...
    With HostApplicationState.Manage(AppState_DisplayAlerts)
        '...do more stuff... (calc and repaints are still disabled)
    End With '<~ alerts are back on here
    '...do stuff...
End With '<~ calc and repaints are re-enabled here (assuming "enabled" initial state)

Of course the biggest caveat with such a class with a side-effecting Terminate handler is that you want to limit its lifetime as much as possible (With blocks are perfect for this!), so the ultimate misuse of that class would be to have a module-scope variable of that type.

Lastly, note that there is no need for a Boolean value to track whether state is enabled or not: that "active" state is made explicit by the caller using a With block to withold the object instance. In other words if the object is alive, it is also active: the caller is in charge of the object's lifetime, and therefore of exactly when the application state gets toggled off and/or back on.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Side note: the IAppState interface isn't really needed [anymore; it was at some point in the refactoring process], but it does help clarifying the API by exposing the initial state to the caller and hiding members that shouldn't be invoked after the object is created and returned. \$\endgroup\$ Jun 21 '21 at 21:04
  • 1
    \$\begingroup\$ Re-reading again, the manual .Toggle call when the class is explicitly New'd up is problematic, because the state is getting encapsulated in an instance that promptly gets discarded... so more work is needed to actually make it work nicely that way - anyway the With block and factory method approach is preferred, since it makes the instance's lifetime explicit. \$\endgroup\$ Jun 22 '21 at 14:09
  • \$\begingroup\$ I learned a lot - as always. I hadn't thought of fast code and the readability (or lack thereof) of Booleans in the perspective you put them. The State Corruption and your alternative are extremely valuable to me - I'll continue to research on these (still not quite there!). I can't thank you enough for the valuable review (much like all the previous)! \$\endgroup\$ Jun 23 '21 at 12: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.