HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

(^|)(Get|Total|Aggregated|Summary|Data|Report) - #NamingIsHard

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
totalnamingishardreportgetsummaryaggregateddata

Problem

The following is the top-level procedure for aggregating, analysing and reporting my company's submitted business from 2015 - present. Reviewing the entire project is unfeasible, so I am just seeking opinions on Variable/Method naming and general readability.

For reference, CLS_Data_Report is a class I built specifically to handle 2-dimensional data array operations (adding, merging, filtering, printing etc.).

Based just on the following, is it clear what the various parts of the procedure are doing?

Are there any code smells?

Is the naming too repetitive?

```
Public Sub GenerateDirectorsReport()

StoreApplicationSettings
DisableApplicationSettings

Set WbSubsheet2015 = GetWorkbook(SUBSHEET_2015_FILENAME, SUBSHEET_2015_FILEPATH)
Set WbSubsheet2016 = GetWorkbook(SUBSHEET_2016_FILENAME, SUBSHEET_2016_FILEPATH)

Dim aggregatedSubsheetDataReport As CLS_Data_Report
Set aggregatedSubsheetDataReport = GetAggregatedSubsheetData(WbSubsheet2015, WbSubsheet2016)
wsAggregatedData.Cells.Clear
aggregatedSubsheetDataReport.PrintToWorksheet wsAggregatedData.Cells(1, 1)

'/============================================================================================================================================================

Dim ws As Worksheet
Set ws = wsSummaryReports

ws.Cells.Clear

Dim printRow As Long, printCol As Long
printRow = 2
printCol = 2

Dim clientInvestmentReport As CLS_Data_Report
Set clientInvestmentReport = GetClientInvestmentReport(aggregatedSubsheetDataReport)
clientInvestmentReport.PrintToWorksheet ws.Cells(printRow, printCol)
FormatPrintedAdviserReport clientInvestmentReport.printRange

printRow = ws.Cells(Rows.Count, printCol).End(xlUp).row + 2

Dim totalOngoingReport As CLS_Data_Report
Set totalOngoingReport = GetTotalOngoingReport(aggregatedSubsheetDataReport)
totalOngoingReport.PrintToWorksheet ws.Cells(printRow, printCol)
FormatPrintedAdviserReport

Solution

Is the naming too repetitive? Let's take a look at your variables

Dim aggregatedSubsheetDataReport As CLS_Data_Report
Dim clientInvestmentReport As CLS_Data_Report
Dim commissionReport As CLS_Data_Report
Dim investmentCodenameStrings As Collection
Dim investmentDataReport As CLS_Data_Report
Dim jeremySmithAdviserReport As CLS_Data_Report
Dim jeremySmithNameStrings As Collection
Dim jonathanBlairAdviserReport As CLS_Data_Report
Dim jonathanBlairNameStrings As Collection
Dim jonHusseyAdviserReport As CLS_Data_Report
Dim jonHusseyNameStrings As Collection
Dim martinCotterAdviserReport As CLS_Data_Report
Dim martinCotterNameStrings As Collection
Dim ongoingCodenameStrings As Collection
Dim printRow As Long, printCol As Long
Dim totalOngoingDataReport As CLS_Data_Report
Dim totalOngoingReport As CLS_Data_Report
Dim ws As Worksheet


Seems repetitive, but it's also pretty clear. I'd drop the strings out of the names and drop the adviser as well.

You might want to make a better distinction between totalOngoingReport and totalOngoingDataReport

and maybe deal with that little ws variable

Let me take a look at the names of the calls you're making

CloseWorkBook
CreateAndPrintAllProviderReports
FormatPrintedAdviserReport
FormatPrintedCommissionReport
.PrintToWorksheet


Not very repetitive there either, even combining with the other variables. It probably looks a little repetitive or cluttered - but I really don't think it is.

Code Snippets

Dim aggregatedSubsheetDataReport As CLS_Data_Report
Dim clientInvestmentReport As CLS_Data_Report
Dim commissionReport As CLS_Data_Report
Dim investmentCodenameStrings As Collection
Dim investmentDataReport As CLS_Data_Report
Dim jeremySmithAdviserReport As CLS_Data_Report
Dim jeremySmithNameStrings As Collection
Dim jonathanBlairAdviserReport As CLS_Data_Report
Dim jonathanBlairNameStrings As Collection
Dim jonHusseyAdviserReport As CLS_Data_Report
Dim jonHusseyNameStrings As Collection
Dim martinCotterAdviserReport As CLS_Data_Report
Dim martinCotterNameStrings As Collection
Dim ongoingCodenameStrings As Collection
Dim printRow As Long, printCol As Long
Dim totalOngoingDataReport As CLS_Data_Report
Dim totalOngoingReport As CLS_Data_Report
Dim ws As Worksheet
CloseWorkBook
CreateAndPrintAllProviderReports
FormatPrintedAdviserReport
FormatPrintedCommissionReport
.PrintToWorksheet

Context

StackExchange Code Review Q#122578, answer score: 3

Revisions (0)

No revisions yet.