5
\$\begingroup\$

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:

    1. Delete

    2. Duplicate (create a new style based on another)

    3. Replace (one style with another)

enter image description here

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

Link to the current file

asked Oct 29, 2019 at 11:13
\$\endgroup\$
3
  • \$\begingroup\$ Maybe this is an overkill way to manage styles - only if you consider VBA a "toy language" that doesn't do OOP ;-) \$\endgroup\$ Commented Oct 29, 2019 at 12:42
  • \$\begingroup\$ You're right @MathieuGuindon question updated ;-) \$\endgroup\$ Commented Oct 29, 2019 at 14:39
  • \$\begingroup\$ Posted a follow up question here \$\endgroup\$ Commented Oct 30, 2019 at 0:13

1 Answer 1

4
\$\begingroup\$

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 Newing 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, and ByVal modifiers.
  • Lots of parameters are implicitly passed ByRef, some are implicitly passed ByVal. Consistency! You want everything passed ByVal unless the compiler says otherwise, or unless you're using ByRef to return values to the caller.
  • Public Property Set Style(ByRef Style As Style) is a lie. Property Set and Property Let procedures always receive their value argument ByVal: the modifier is not only not needed, it's outright lying. And since the default/implicit modifier is ByRef 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 how ByRef/ByVal work.
  • Vertical whitespace in Duplicate is distracting.
  • cStyles.Item wants a @DefaultMember annotation (/VB_UserMemId = 0 attribute).
  • The LoadStyles and ProcessStyles macros don't need a local variable; just go With New cStyles and perform the member calls against the With block variable.
  • Both LoadStyles and ProcessStyles are implicitly Public.
  • 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.

answered Oct 29, 2019 at 16:50
\$\endgroup\$
2
  • \$\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\$ Commented 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\$ Commented Oct 29, 2019 at 23:30

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.