Creating a Chart Worksheet from scratch with VBA / OOP Design

Question

I recently posted some code for review, and am now looking to gather some feedback on my latest implementation with that code. Yes, I have fallen into the OOP rabbit hole (thanks Mathieu Guindon) and am curious if my OOP approach is on the right track.

Some background: I am creating a Chart Workbook from scratch via data that is generated from a Bot at work. Basically I take the data from the Bot generated Workbook, store it into an Array and then use a Scripting Dictionary to sort out all the duplicates, this approach works great! My code below is merely just the Worksheet part of my project, and am only at the point of creating the Headers for said chart.

Am I on the right track so far?

IChartFormatService

My hope was to separate this concern from my ChartWorksheet class. Why would I want to do that? In the future I may need to implement different styles for different reasons, based on customer / work needs. I can have a particular Worksheet implement a particular flavor of colors for different representations.

'@Interface
Option Explicit

Public Sub FormatProductHeaderLabel()
End Sub

Public Sub FormatServiceHeaderLabel()
End Sub

Here is one implementation:

StandardChartWorkSheet

'@PredeclaredId
Option Explicit

Implements IChartFormatService
Implements IChart

Private Const ProductHeaderFont As String = "Arial"
Private Const ProductHeaderFontSize As Integer = 12
Private Const ProductHeaderFontColor As Long = 16777215

Private Const ServiceHeaderFont As String = "Arial"
Private Const ServiceHeaderFontSize As Integer = 10
Private Const ServiceHeaderFontColor As Long = 0

Public Enum ChartColor
    InteriorProductColumnColor = 12549120
    InteriorServiceColumnColor = 14277081
End Enum

Private Type TChartWorksheetService
    HeaderColumn As Long
    HeaderData As Scripting.Dictionary
    ChartWorksheet As Worksheet
End Type

Private this As TChartWorksheetService

Public Function Create(ByVal hData As Scripting.Dictionary, cSheet As Worksheet) As IChart
    With New StandardChartWorksheet
        Set .HeaderData = hData
        Set .ChartWorksheet = cSheet
        Set Create = .Self
    End With
End Function

Public Property Get HeaderData() As Scripting.Dictionary
    Set HeaderData = this.HeaderData
End Property

Public Property Set HeaderData(ByVal value As Scripting.Dictionary)
    Set this.HeaderData = value
End Property

Public Property Get ChartWorksheet() As Worksheet
    Set ChartWorksheet = this.ChartWorksheet
End Property

Public Property Set ChartWorksheet(ByVal value As Worksheet)
    Set this.ChartWorksheet = value
End Property

Public Property Get HeaderColumn() As Long
    HeaderColumn = this.HeaderColumn
End Property

Public Property Let HeaderColumn(ByVal value As Long)
    this.HeaderColumn = value
End Property

Public Property Get Self() As IChart
    Set Self = Me
End Property

Private Sub BuildHeaders()
    Application.ScreenUpdating = False
    Dim product As Variant
    For Each product In HeaderData
        PrintProductValues product
        this.HeaderColumn = this.HeaderColumn + 1
        Dim service As Variant
        For Each service In HeaderData(product)
            PrintServiceValues service
            this.HeaderColumn = this.HeaderColumn + 1
        Next
    Next
    Application.ScreenUpdating = True
End Sub

Private Sub PrintProductValues(ByVal product As String)
    With this.ChartWorksheet.Range(Cells(4, this.HeaderColumn), Cells(50, this.HeaderColumn))
        .Interior.Color = InteriorProductColumnColor
    End With

    With Sheet3.Cells(4, this.HeaderColumn)
        .value = product
        IChartFormatService_FormatProductHeaderLabel
    End With
End Sub

Private Sub PrintServiceValues(ByVal service As String)
    With this.ChartWorksheet.Cells(4, this.HeaderColumn)
        .value = Mid(service, 14, 100)
        IChartFormatService_FormatServiceHeaderLabel
    End With
End Sub

Private Sub IChartFormatService_FormatProductHeaderLabel()
    With this.ChartWorksheet.Cells(4, this.HeaderColumn)
        .Font.Name = ProductHeaderFont
        .Font.Size = ProductHeaderFontSize
        .Font.Color = ProductHeaderFontColor
        .Font.Bold = True
        .Orientation = Excel.XlOrientation.xlUpward
        .Columns.AutoFit
    End With
End Sub

Private Sub IChartFormatService_FormatServiceHeaderLabel()
    With this.ChartWorksheet.Cells(4, this.HeaderColumn)
        .Interior.Color = InteriorServiceColumnColor
        .Font.Name = ServiceHeaderFont
        .Font.Size = ServiceHeaderFontSize
        .Font.Bold = False
        .Font.Color = ServiceHeaderFontColor
        .Orientation = Excel.XlOrientation.xlUpward
        .Columns.AutoFit
    End With
End Sub

Private Sub IChart_BuildChart()
    If Not this.HeaderData Is Nothing Then
        BuildHeaders
    Else: Exit Sub
    End If
End Sub

Private Sub Class_Initialize()
    this.HeaderColumn = 3
End Sub

StandardChartWorksheet class implements another interface, IChart basically separating the concern of building a chart

'@Interface
Option Explicit

Public Sub BuildChart()
End Sub

My sample procedure, housed in Module 1

Sub test()
    Dim chart As IChart
    Set chart = StandardChartWorksheet.Create(GetTMProductDictionary, Sheet3)
    
    chart.BuildChart
End Sub

Snippet of what's produced

There are 50 more columns, cropped the picture to keep it simple.

Answer 1

Naming

I would change StandardChartWorkSheet to StandardChart to avoid an ambiguity with a Chart sheet.

The Print prefix implies printing to the debug window. Add makes more sense to me (e.g AddProductValues()`.

ByVal value As Scripting.Dictionary Value should be capitalized because it is a common property and the VBE changes case of variables to match the last declaration using with that name. This will prevent confusion when reading and writing code. You don't want to see cell.value when you are expecting cell.Value.

TChartWorksheetService

I prefer to use this instead of Matt's Self(). In any case, this implies a reference to the actual class.

Mathieu Guindon likes to wrap the private fields (members) of his classes in a Type and name the Type T + ClassName. This is an awesome idea but I prefer to standardize the names whenever possible. The Type that holds the private fields of my class are always named Members and I always name my reference variable m (this is similar to the VBA class field convention that prefixes class variables with m.

Don't get me wrong Matt knows 10 times more than I do about coding than I do. But when I am review a class if I see TChartWorksheetService I have to stop think what is TChartWorksheetService. Is it a built in or custom class? Oh wait, its a Type so it can't be passed into a class. How is it used? Where is it used? Where as I see private m As Members, I think oh private fields and move on.

Properties and Constants

Constants values are great for storing magic numbers and immutable strings but are constants really what you need here? If a user needs an Arial ServiceHeaderFont on one worksheet and a Times New Roman ServiceHeaderFont on another then you will have to write two different classes or worse yet (what usually happens) you write a hack routine to make the StandardChartWorkSheet fit the new specifications. Can you imagine having to have an ArialTexbox and TimesNewRomanTextBox...ugh. It would be better to define most of these settings as properties of the IChart and assign the default values in your factory methods.

For example:

IChart:

Option Explicit

Public Sub BuildChart()
End Sub

Public Property Get ProductHeaderFont() As String
End Property

Public Property Let ProductHeaderFont(ByVal Value As String)
End Property

Public Property Get ProductHeaderFontSize() As Single
End Property

Public Property Let ProductHeaderFontSize(ByVal Value As Single)
End Property

'***** More settings *******

StandardChartWorkSheet

AsIChart() was added to make it easier to reference the class as StandardChartWorkSheet class.

Private mProductHeaderFont As String
Private mProductHeaderFontSize As Integer

Public Function Create(ByVal hData As Scripting.Dictionary, cSheet As Worksheet) As IChart
    With New StandardChartWorkSheet
        Set .HeaderData = hData
        Set .ChartWorksheet = cSheet
        Set Create = .Self
        With .AsIChart
            .ProductHeaderFont = ProductHeaderFont
            .ProductHeaderFontSize = ProductHeaderFontSize
        End With
    End With
End Function

Public Function AsIChart() As IChart
    Set GetIChartFromClass = Self
End Function

Private Property Let IChart_ProductHeaderFont(ByVal RHS As String)
    mProductHeaderFont = RHS
End Property

Private Property Get IChart_ProductHeaderFont() As String
    IChart_ProductHeaderFont = mProductHeaderFont
End Property

Private Property Let IChart_ProductHeaderFontSize(ByVal RHS As Single)
    mProductHeaderFontSize = RHS
End Property

Private Property Get IChart_ProductHeaderFontSize() As Single
    IChart_ProductHeaderFontSize = mProductHeaderFontSize
End Property

Sub NewTest()
    Dim chart As IChart
    Set chart = StandardChartWorkSheet.Create(GetTMProductDictionary, Sheet3)
    chart.ProductHeaderFont = "Times New Roman"
    chart.ProductHeaderFontSize = 14
    chart.BuildChart
End Sub

IChartFormatService

If the VBA supported polymorphism, I would tell you that IChartFormatService should be an abstract class because it is only used internally by the StandardChartWorkSheet class. Interfaces are meant to be used to expose methods of the class not just to enforce implementation of a method. IMO IChartFormatService is just decoration. I would drop it because I don't want to have to port it the next project I need a StandardChartWorkSheet.

BuildHeaders

Application.ScreenUpdating = True is no longer necessary. ScreenUpdating will automatically resume after all the code has ran. This was changed in either Excel 2007 or 2010. If you are worried about backwards compatibility then you should save and restore the Application.ScreenUpdating state. This will prevent slow downs when running multiple procedures.

PrintProductValues

With Sheet3.Cells(4, this.HeaderColumn) is a refactoring over site.

Create()

Referencing the TopLeftCell that you want to target will allow you to add multiple chart to the same worksheet.

Public Function Create(ByVal hData As Scripting.Dictionary, TopLeftCell As Range) As IChart

HeaderColumn

CurrentHeaderColumn or change HeaderIndex are better names for HeaderColumn.

HeaderColumn should not belong to the class. Class variables are subject to modification by multiple procedures. This makes it far easier to make mistakes and takes longer to read, modify and debug.

If by contrast, you pass the HeaderColumn as a parameter, you will know empirically when and where the value is being modified.

Private Sub PrintProductValues(ByVal product As String, ByVal HeaderColumn As Long)

Miscellaneous

.Value = Mid(service, 14, 100) works perfect and is exactly what you need if you expect values over 100 characters. Otherwise, .Value = Mid(service, 14) will return the same value.

Cells(50, this.HeaderColumn) Why fifty? It seems like this needs to be more dynamic.