I want to manage the Excel Styles in a more flexible way with bulk actions and at the same time improve my newly acquired OOP concepts.
Objectives:
Load the current Styles list (name and type=builtin or custom) in an Excel Structured Table (ListObject)
Allow users to:
Delete
Duplicate (create a new style based on another)
Replace (one style with another)
Main events:
1) Press "Load" button -> Load the workbook (thisworkbook) styles list into the Excel structured table
2) Press "Process" button -> Review each cell of the Source column in the Excel Table and run the selected action
Actions:
When user selects "Delete" -> The Excel Style based on the source column will be deleted
When user selects "Duplicate" -> A new Excel Style should be created with the name defined in the table column "Duplicated new style name | replace with"
When user selects "Replace" -> All instances where the Style is found in the workbook should be replaced with the one defined in the table column "Duplicated new style name | replace with"
Question: How can I structure this classes to add more Actions based on Composition?
Question: Would the Factory Pattern help to make this classes easier to maintain?
(削除) Maybe (削除ここまで) this is (削除) an overkill (削除ここまで) the best way to manage styles, (削除) but (削除ここまで) and I want to do it (削除) as a proof of concept too and (削除ここまで) to learn more about OOP.
Any help reviewing the code I'd appreciate.
Module: mStyles
'@Folder("Styles")
Option Explicit
Sub LoadStyles()
Dim myStyles As cStyles
Set myStyles = New cStyles
myStyles.LoadToTable
End Sub
Sub ProcessStyles()
Dim myStyles As cStyles
Set myStyles = New cStyles
myStyles.LoadFromTable
myStyles.ProcessStyles
myStyles.LoadToTable
End Sub
Class: cStyle
'@Folder("Styles")
Option Explicit
Private Type TStyle
Style As Style
Name As String
Action As String
Target As String
Exists As Boolean
End Type
Private this As TStyle
Public Property Let Name(value As String)
this.Name = value
End Property
Public Property Get Name() As String
Name = this.Name
End Property
Public Property Let Action(value As String)
this.Action = value
End Property
Public Property Get Action() As String
Action = this.Action
End Property
Public Property Let Target(value As String)
this.Target = value
End Property
Public Property Get Target() As String
Target = this.Target
End Property
Public Property Set Style(ByRef Style As Style)
Set this.Style = Style
End Property
Public Property Get Style() As Style
Set Style = this.Style
End Property
Public Sub Init(Name, Action, Target)
this.Name = Name
this.Action = Action
this.Target = Target
If Exists Then
Set this.Style = ThisWorkbook.Styles(this.Name)
End If
End Sub
Public Function Exists() As Boolean
' Returns TRUE if the named style exists in the target workbook.
On Error Resume Next
Exists = Len(ThisWorkbook.Styles(this.Name).Name) > 0
On Error GoTo 0
End Function
Public Function Duplicate() As Style
Dim styleCell As Range
Dim newName As String
Set styleCell = MStyles.Range("E1")
styleCell.Style = this.Name
newName = this.Target
Set Duplicate = ThisWorkbook.Styles.Add(newName, styleCell)
styleCell.Clear
End Function
Public Sub Delete()
If Exists Then this.Style.Delete
End Sub
Public Sub Replace()
Dim evalCell As Range
Dim newStyle As Style
Dim replaceSheet As Worksheet
Set newStyle = ThisWorkbook.Styles(this.Target)
For Each replaceSheet In ThisWorkbook.Worksheets
For Each evalCell In replaceSheet.UsedRange.Cells
If evalCell.Style = this.Style And evalCell.MergeCells = False Then evalCell.Style = newStyle
Next evalCell
Next replaceSheet
End Sub
Class: cStyles
'@Folder("Styles")
Option Explicit
Private Type TStyles
Styles As Collection
End Type
Private this As TStyles
Private Sub Class_Initialize()
Set this.Styles = New Collection
End Sub
Private Sub Class_Terminate()
Set this.Styles = Nothing
End Sub
Public Sub Add(obj As cStyle)
this.Styles.Add obj
End Sub
Public Sub Remove(Index As Variant)
this.Styles.Remove Index
End Sub
Public Property Get Item(Index As Variant) As cStyle
Set Item = this.Styles.Item(Index)
End Property
Property Get Count() As Long
Count = this.Styles.Count
End Property
Public Sub Clear()
Set this.Styles = New Collection
End Sub
Public Sub LoadToTable()
Dim stylesTable As ListObject
Dim curStyle As Style
Dim tempStyleInfo() As Variant
Dim counter As Integer
Dim counterStyles As Integer
counterStyles = ThisWorkbook.Styles.Count
ReDim tempStyleInfo(counterStyles, 3)
Set stylesTable = MStyles.ListObjects("TableStyles")
If Not stylesTable.DataBodyRange Is Nothing Then stylesTable.DataBodyRange.Delete
For Each curStyle In ThisWorkbook.Styles
tempStyleInfo(counter, 0) = curStyle.Name
tempStyleInfo(counter, 1) = IIf(curStyle.BuiltIn, "BuiltIn", "Custom")
counter = counter + 1
Next curStyle
stylesTable.Resize stylesTable.Range.Resize(RowSize:=UBound(tempStyleInfo, 1))
stylesTable.DataBodyRange = tempStyleInfo
End Sub
Public Sub LoadFromTable()
Dim stylesTable As ListObject
Dim styleCell As cStyle
Dim cellStyle As Range
Set stylesTable = MStyles.ListObjects("TableStyles")
For Each cellStyle In stylesTable.DataBodyRange.Columns(1).Cells
If cellStyle.Offset(ColumnOffset:=2) <> "" Then
Set styleCell = New cStyle
styleCell.Init cellStyle.Value2, cellStyle.Offset(ColumnOffset:=2).Value2, cellStyle.Offset(ColumnOffset:=3).Value2
this.Styles.Add styleCell
End If
Next cellStyle
End Sub
Public Sub ProcessStyles()
Dim styleCell As cStyle
For Each styleCell In this.Styles
Select Case styleCell.Action
Case "Delete"
styleCell.Delete
Case "Duplicate"
styleCell.Duplicate
Case "Replace"
styleCell.Replace
End Select
Next styleCell
End Sub
-
\$\begingroup\$ Maybe this is an overkill way to manage styles - only if you consider VBA a "toy language" that doesn't do OOP ;-) \$\endgroup\$Mathieu Guindon– Mathieu Guindon2019年10月29日 12:42:27 +00:00Commented Oct 29, 2019 at 12:42
-
\$\begingroup\$ You're right @MathieuGuindon question updated ;-) \$\endgroup\$Ricardo Diaz– Ricardo Diaz2019年10月29日 14:39:06 +00:00Commented Oct 29, 2019 at 14:39
-
\$\begingroup\$ Posted a follow up question here \$\endgroup\$Ricardo Diaz– Ricardo Diaz2019年10月30日 00:13:37 +00:00Commented Oct 30, 2019 at 0:13
1 Answer 1
Code is generally very clean, although I do have a number of reservations with some of the naming: c
prefix for class modules, M
for standard ones, is pure noise; Cell
as a suffix for something that isn't a cell, is confusing. That kind of thing.
I would have named cStyles
as Styles
, or perhaps StyleProcessor
since we don't want to hide Excel.Styles
; anything that makes it sound like it's more than just a custom collection of styles would probably be a good name. MStyles
is confusing - I'd just call it Macros
, since all it contains is, well, macros (/entry-point procedures), and too many things are "styles" here.
The internal Private Type
isn't being useful here. If there was a Styles
property, it would be. But there isn't, so it's not helping with any name-clashing properties/private fields.
The cStyle
class, I'd probably name it StyleConfig
, or StyleInfo
- plain Style
would be hiding Excel.Style
, and we would rather avoid doing that. If we go with StyleInfo
, then infos
makes a reasonably sensible name for it:
Private infos As Collection
A Factory Pattern doesn't directly make code easier to maintain. In fact it could be argued that it makes things more complicated! Dependency Injection in VBA explains where and why you would want to use a factory pattern: it's a tool to help reduce coupling. In itself, a factory method is little more than an Init
method that, instead of initializing the current instance, creates, initializes, and returns a new one - effectively allowing parameterized initialization of objects, like constructors do in other languages.
Having a factory method on cStyle
(with a default instance / predeclared ID) would remove the need to have an Init
method, so you could do this:
this.Styles.Add cStyle.Create(...)
A factory method can't really hurt, but a factory pattern would indeed be overkill: you don't need to decouple cStyle
from cStyles
, the two classes are tightly coupled, but unless you're looking to decouple the Excel.Style
dependency, there's little to gain here IMO.
Question: How can I structure this classes to add more Actions based on Composition?
You'd have to extract an IAction
(or IStyleAction
) class/interface, and implement it with some DeleteStyleAction
, DuplicateStyleAction
, and ReplaceStyleAction
classes, and then ProcessStyles
(I'd trim it to just Process
) starts looking very much like a strategy pattern:
Public Sub Process()
Dim info As StyleInfo
For Each info In infos
Dim strategy As IStyleAction
Set strategy = styleActions(info.Action)
strategy.Run
Next
End Sub
Where IStyleAction
is a class/interface stub exposing a simple Run
method, and styleActions
could be a simple keyed collection:
Private Sub Class_Initialize()
Set infos = New Collection
Set styleActions = New Collection
styleActions.Add New DeleteStyleAction, "Delete"
styleActions.Add New DuplicateStyleAction, "Duplicate"
styleActions.Add New ReplaceStyleAction, "Replace"
'...
End Sub
Notice how every single one of these New
statements increases the number of classes that are coupled with this StyleProcessor
(cStyles
) class? That's because the StyleProcessor
is responsible for knowing what actions are available and what string value refers to what action - if we removed that responsibility, we would also remove that coupling. We can remove responsibilities from a class by injecting components instead of New
ing them up. See Dependency Injection in VBA if that's something you want to explore.
Other observations, in no particular order:
cStyle.Init
needs explicit declared types for the parameters, andByVal
modifiers.- Lots of parameters are implicitly passed
ByRef
, some are implicitly passedByVal
. Consistency! You want everything passedByVal
unless the compiler says otherwise, or unless you're usingByRef
to return values to the caller. Public Property Set Style(ByRef Style As Style)
is a lie.Property Set
andProperty Let
procedures always receive their value argumentByVal
: the modifier is not only not needed, it's outright lying. And since the default/implicit modifier isByRef
anyway, I'm worried this one was added "because it's an object and so it must be passed by reference" (not true), which denotes a misunderstanding of howByRef
/ByVal
work.- Vertical whitespace in
Duplicate
is distracting. cStyles.Item
wants a@DefaultMember
annotation (/VB_UserMemId = 0
attribute).- The
LoadStyles
andProcessStyles
macros don't need a local variable; just goWith New cStyles
and perform the member calls against theWith
block variable. - Both
LoadStyles
andProcessStyles
are implicitlyPublic
. - Not sure
Clear
has any business being exposed; feels like YAGNI (You Ain't Gonna Need It).
Rubberduck inspections should be warning you about the implicit modifiers and unused members.
-
\$\begingroup\$ Absolutely amazing. Whole day spent trying to put in practice your reviews. Thank you Mathieu! Just one question: if I want to review the results that incorporate your comments and suggestions should I post another question or edit this one? \$\endgroup\$Ricardo Diaz– Ricardo Diaz2019年10月29日 23:26:32 +00:00Commented Oct 29, 2019 at 23:26
-
\$\begingroup\$ @RicardoDiaz you can absolutely post a follow-up question; please avoid editing code in answered questions, such (answer-invalidating) edits are monitored and will be rolled back =) \$\endgroup\$Mathieu Guindon– Mathieu Guindon2019年10月29日 23:30:32 +00:00Commented Oct 29, 2019 at 23:30
Explore related questions
See similar questions with these tags.