patternMinor
(^|)(Get|Total|Aggregated|Summary|Data|Report) - #NamingIsHard
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,
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
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
Seems repetitive, but it's also pretty clear. I'd drop the
You might want to make a better distinction between
and maybe deal with that little
Let me take a look at the names of the calls you're making
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.
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 WorksheetSeems 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 totalOngoingDataReportand maybe deal with that little
ws variableLet me take a look at the names of the calls you're making
CloseWorkBook
CreateAndPrintAllProviderReports
FormatPrintedAdviserReport
FormatPrintedCommissionReport
.PrintToWorksheetNot 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 WorksheetCloseWorkBook
CreateAndPrintAllProviderReports
FormatPrintedAdviserReport
FormatPrintedCommissionReport
.PrintToWorksheetContext
StackExchange Code Review Q#122578, answer score: 3
Revisions (0)
No revisions yet.