Very often on Stack Overflow, and even on Code Review, I've seen questions and answers that have code that begins by persisting Excel properties like DisplayAlerts
and ScreenUpdating
to public or private variables, and the restores them at the end of the procedure.
Public bScreenUpdating As Boolean
Public bEnableEvents As Boolean
Public xlCalc As XlCalculation
Public Sub PersistAppSettings()
bScreenUpdating = Application.ScreenUpdating
bEnableEvents = Application.EnableEvents
xlCalc = Application.Calculation
End Sub
Public Sub DisableAppSettings()
With Application
.ScreenUpdating = False
.EnableEvents = False
.Calculation = xlCalculationManual
End With
End Sub
Public Sub RestoreAppSettings()
With Application
.ScreenUpdating = bScreenUpdating
.EnableEvents = bEnableEvents
.Calculation = xlCalc
End With
End Sub
That works well enough when the procedure is viewed in isolation, but it mightn't work very well once the call stack gets deeper. That's potentially a lot of private variables to declare and assign, and it can quickly get confusing about when and where properties are set and unset.
Also, there are at least 4 properties that are regularly persisted, but usually only 3 of the 4 are persisted. Namely:
Calculation, DisplayAlerts, EnabledEvents and ScreenUpdating
Furthermore, if the restore procedure is never called, due to an error or code branching, then the settings are never restored.
The solution? Use a class. I've written a class, and a test harness that shows how it can be used in a stack, and without explicitly restoring the properties. The class exposes properties that allow you to disable certain Excel properties from being restored, and a series of constants allow you to define the default behaviour of the class.
I'd appreciate a review of the class and the test harness. Are there other Excel Properties that could be added?
Class Module: CExcelProperties
'These constants define the default restoration behaviors for the class
Private Const DEFAULT_RESTORE_CALCULATION = True
Private Const DEFAULT_RESTORE_DISPLAY_ALERTS = True
Private Const DEFAULT_RESTORE_ENABLE_EVENTS = True
Private Const DEFAULT_RESTORE_SCREEN_UPDATING = True
'Set this to true to ensure a persisted state is restored, even if the consumer forgets to restore
Private Const RESTORE_ON_TERMINATE = True
'Private members stored in a Type
Private this As TMembers
Private Type TMembers
Calculation As XlCalculationState
DisplayAlerts As Boolean
EnableEvents As Boolean
ScreenUpdating As Boolean
RestoreCA As Boolean
RestoreDA As Boolean
RestoreEE As Boolean
RestoreSU As Boolean
IsPersisted As Boolean
IsRestored As Boolean
End Type
'Set the default restoration behaviours on intialize
Private Sub Class_Initialize()
this.RestoreCA = DEFAULT_RESTORE_CALCULATION
this.RestoreDA = DEFAULT_RESTORE_DISPLAY_ALERTS
this.RestoreEE = DEFAULT_RESTORE_ENABLE_EVENTS
this.RestoreSU = DEFAULT_RESTORE_SCREEN_UPDATING
End Sub
'By default, restore the settings if we didn't do it explicitly
Private Sub Class_Terminate()
If this.IsPersisted And Not this.IsRestored And RESTORE_ON_TERMINATE Then
Me.Restore
End If
End Sub
Public Property Get RestoreCalculation() As Boolean
RestoreCalculation = this.RestoreCA
End Property
Public Property Let RestoreCalculation(Value As Boolean)
this.RestoreCA = Value
End Property
Public Property Get RestoreDisplayAlerts() As Boolean
RestoreDisplayAlerts = this.RestoreDA
End Property
Public Property Let RestoreDisplayAlerts(Value As Boolean)
this.RestoreDA = Value
End Property
Public Property Get RestoreEnableEvents() As Boolean
RestoreEnableEvents = this.RestoreEE
End Property
Public Property Let RestoreEnableEvents(Value As Boolean)
this.RestoreEE = Value
End Property
Public Property Get RestoreScreenUpdating() As Boolean
RestoreScreenUpdating = this.RestoreSU
End Property
Public Property Let RestoreScreenUpdating(Value As Boolean)
this.RestoreSU = Value
End Property
Public Sub Save()
If Not this.IsPersisted Then
'Save all of the settings
With Application
this.Calculation = .Calculation
this.DisplayAlerts = .DisplayAlerts
this.EnableEvents = .EnableEvents
this.ScreenUpdating = .ScreenUpdating
End With
this.IsPersisted = True
Else
Err.Raise -1000, "CExcelProperties", "Properties have already been persisted."
End If
End Sub
Public Sub Restore()
'Only restore the settings that we want restored
'(which by default is all of them)
With Application
If this.RestoreCA Then
.Calculation = this.Calculation
End If
If this.RestoreDA Then
.DisplayAlerts = this.DisplayAlerts
End If
If this.RestoreEE Then
.EnableEvents = this.EnableEvents
End If
If this.RestoreSU Then
.ScreenUpdating = this.ScreenUpdating
End If
End With
this.IsRestored = True
End Sub
Standard Module:
Sub TestHarness()
Dim cProps As CExcelProperties
Set cProps = New CExcelProperties
'Save the current properties
cProps.Save
'Adjust the Application properties
Application.DisplayAlerts = False
'Run something that further adjusts the Application properties
Call SubProcess
cProps.Restore
End Sub
Sub SubProcess()
Dim cProps As CExcelProperties
Set cProps = New CExcelProperties
'Save the current properties
cProps.Save
Application.DisplayAlerts = True
'Limit the restoration to exclude calculation
cProps.RestoreCalculation = False
'Deliberately don't restore, and the terminate event should restore the properties for us
End Sub
1 Answer 1
I like the idea very much - encapsulating this concern in its own class is very OOP and certainly helps writing cleaner VBA.
I like that you're wrapping all fields in a private type, leaving only this
as a private field, which makes implementing (and more importantly, reading!) the properties a pleasure. I would have made the private type's member names match the properties identically though: RestoreCA
and the other abbreviated members feel a bit sloppy, there's not really a reason to not spell them out completely. Doing so would have been perfectly consistent.
I'm not sure why RESTORE_ON_TERMINATE
is a constant rather than a configurable property though; code shouldn't need to change, especially not for changing a constant value. Rename it to DEFAULT_RESTORE_ON_TERMINATE
and add the corresponding members, and you're never going to need to change it.
I think I would nest the condition in Class_Terminate
, to check for this.RestoreOnTerminate
and skip the other checks entirely when it's false
.
I don't think any of the comments provide any value - your code is clear and readable, and speaks for itself already; the comments are somewhat redundant, I'd remove them.
The name Save
isn't consistent with the terminology you're using, I'd rename it to Persist
... but either is somewhat confusing, since you're not physically persisting anything, merely remembering values for the lifetime of the object - but Remember
would be even more awkward, so you're forgiven ;-)
The With
blocks are fine, but I wouldn't have them... but this might only be because I dislike additional nesting levels more than repetitive/explicit calls to the same object. Or it could be my C# eyes being used to such explicit calls. Or it could be just me not liking the VB With
keyword at all - in that case don't mind me, your code is fine.
I don't like the hard-coded -1000 error number there. A smart little ducky once showed me a neat little trick that stuck with me:
Public Enum ExcelPropertiesError
Error_AlreadyInitialized = vbObjectError + 42
End Enum
And now you can do this:
Err.Raise Error_AlreadyInitialized '...
And the client code that's handling it can use the enum instead of hard-coding error numbers everywhere, and if you have more errors to raise, you just add an enum member and VBA automatically makes it vbObjectError + 43
:-)
Your tests are insufficient. Get Rubberduck and write actual unit tests that cover all execution paths: you want one test to cover one thing, or rather, you want each test to have one reason to fail - and more importantly you want your tests to fail when something changes in the class under test, that changes the behavior that the test is documenting.
For example, you'll want a test that fails when Error_AlreadyInitialized
isn't raised when you call Save
twice.
You'll want a test for each property you're persisting, that fails when the getter doesn't return the value that was persisted; you'll want a test for each getter again, that fails when it doesn't return the expected value after it was saved and then modified.
Also, you'll want a test that covers this nasty gotcha:
Sub DoSomething()
Dim props As New CExcelProperties
props.Save
Application.DisplayAlerts = False
'does it restore?
End Sub
You'll also want to test what happens when more than once instance is alive at different depths in the call stack; you might want to consider making the class have a PredeclaredId
and living inside a separate project than the client code that uses it, so that is can't be instantiated and every call is made on the same, default instance - how would the Terminate
handler deal with that?
Nitpicks:
Call
is obsolete, forget it even exists. Use the modern implicit call syntax for calling functions and procedures instead:SubProcess 'instead of Call SubProcess
The
value
parameter of each getter is implicitly passedByRef
, and should be passedByVal
. That's something Rubberduck lets you fix with a single click.
-
\$\begingroup\$ The constants were designed so that a developer could alter the default behaviors without having to edit any code. It's logical, for my workload, to save and restore the calculation property, but it mightn't be a property that you want to change, or want to have to turn off every time, so the constants were a way of making the class adaptable to a developer's preferred default behavior. I like your point about the Restore on Terminate being exposed as a property. I actually added the Terminate event in the CR textbox, so I wasn't in IDE/OOP mentality when I added it. \$\endgroup\$ThunderFrame– ThunderFrame2016年01月17日 08:57:36 +00:00Commented Jan 17, 2016 at 8:57
-
\$\begingroup\$ Likewise the members of the this type, and their usage, were typed in the CR textbox, so abbreviating them was easier than typing them in full without Intellisense. So yes, the Type members should match the property names. \$\endgroup\$ThunderFrame– ThunderFrame2016年01月17日 09:00:54 +00:00Commented Jan 17, 2016 at 9:00
-
\$\begingroup\$ I've used your error numbering approach in my own implementation, but I wanted to keep the example focused on the persistence. Adding in a bunch of error handling is necessary, but I didn't want it to clutter the code. \$\endgroup\$ThunderFrame– ThunderFrame2016年01月17日 09:03:08 +00:00Commented Jan 17, 2016 at 9:03
-
\$\begingroup\$ I believe your nasty Initialize gotcha is the issue I added to RubberDuck earlier! \$\endgroup\$ThunderFrame– ThunderFrame2016年01月17日 09:15:53 +00:00Commented Jan 17, 2016 at 9:15
-
2\$\begingroup\$ Ideally you put up your actual code for review, so as to avoid "yes but my real code does X" replies ;-) ...and yeah, that gotcha is something Rubberduck should warn about; I'm just saying, tests should document a class' behavior/specs, so when all tests pass you know it's working as intended, and you know what the intent is just by looking at test names, not by looking at the class' implementation. \$\endgroup\$Mathieu Guindon– Mathieu Guindon2016年01月17日 14:18:19 +00:00Commented Jan 17, 2016 at 14:18
Application.Visible
as well. I change that one from time to time and would benefit from it resetting correctly if the code fails. \$\endgroup\$Application.DisplayFormulaBar
\$\endgroup\$