# Creating a Chart Worksheet from scratch with VBA / OOP Design

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

End Sub

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
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 .ChartWorksheet = cSheet
Set Create = .Self
End With
End Function

Public Property Get HeaderData() As Scripting.Dictionary
End Property

Public Property Set HeaderData(ByVal value As Scripting.Dictionary)
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
End Property

Public Property Let HeaderColumn(ByVal value As Long)
End Property

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

Application.ScreenUpdating = False
Dim product As Variant
PrintProductValues product
Dim service As Variant
PrintServiceValues service
Next
Next
Application.ScreenUpdating = True
End Sub

Private Sub PrintProductValues(ByVal product As String)
.Interior.Color = InteriorProductColumnColor
End With

.value = product
End With
End Sub

Private Sub PrintServiceValues(ByVal service As String)
.value = Mid(service, 14, 100)
End With
End Sub

.Font.Bold = True
.Orientation = Excel.XlOrientation.xlUpward
.Columns.AutoFit
End With
End Sub

.Interior.Color = InteriorServiceColumnColor
.Font.Bold = False
.Orientation = Excel.XlOrientation.xlUpward
.Columns.AutoFit
End With
End Sub

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

Private Sub Class_Initialize()
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.

## 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

Public Function Create(ByVal hData As Scripting.Dictionary, cSheet As Worksheet) As IChart
With New StandardChartWorkSheet
Set .ChartWorksheet = cSheet
Set Create = .Self
With .AsIChart
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)
End Property

Private Property Get IChart_ProductHeaderFont() As String
End Property

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

Private Property Get IChart_ProductHeaderFontSize() As Single
End Property

Sub NewTest()
Dim chart As IChart
Set chart = StandardChartWorkSheet.Create(GetTMProductDictionary, Sheet3)
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.

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


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.

• thank you for your honest review! I'll admit probably the most challenging part of OOP for me so far is the naming of variables / creating meaningful names with value. I hear you about setting the font names via Factory however I only need to implement 1 chart per Sheet. With that said yes, I would be creating a whole new Class for say a QuarterlyChart or a YearEndChat. I think it would be much cleaner at that point, for each Chart Class would have its own logic, and methods with fonts and colors. – Jose Cortez Jul 22 '20 at 21:13
• Have you given any thought to how you are going to populate the chart? – TinMan Jul 22 '20 at 21:27
• @JoseCortez you should read this answer to views Excel Data Import and Manipulation, too slow?. He lays down the principles for writing solid code. Basically, everything I said but in a generic way. So much better than mine. – TinMan Jul 26 '20 at 15:08
• thanks for providing the link! I'm going to take time to read it. Don't sell yourself short, you provided a similar best practices in my previous post regarding Product Dictionary's ;) – Jose Cortez Jul 26 '20 at 17:20
• @TinMan, I'm having trouble following. Seeing With .AsIChart (thought Whaaat? Undocumented feature?) but then no, AsIChart is a function. But Public Function AsIChart() As IChart doesn't set AsIChart but does Set GetIChartFromClass = Self. Guessing GetIChartFromClass` is a member that didn't get included in the paste? – klausnrooster Oct 22 '20 at 21:54