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

\$\endgroup\$
  • \$\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 Oct 29 at 12:42
  • \$\begingroup\$ You're right @MathieuGuindon question updated ;-) \$\endgroup\$ – Ricardo Diaz Oct 29 at 14:39
  • \$\begingroup\$ Posted a follow up question here \$\endgroup\$ – Ricardo Diaz Oct 30 at 0:13
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.

\$\endgroup\$
  • \$\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 Oct 29 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 Oct 29 at 23:30

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.