patternMinor
Select Case w/Public Functions
Viewed 0 times
caseselectfunctionspublic
Problem
I am looking to enhance my VBA skills and efficiency with the code below. It currently executes at about 20 minutes. I just recently learned how to use public functions but this project is very robust and I haven't been able to make it more efficient. I've cut off two Case requirements
```
Dim Wb1 As Workbook, Wb2 As Workbook
Dim Wk4 As Worksheet, Wk5 As Worksheet, Wk7 As Worksheet
Dim Wk1 As Worksheet
Dim CCRg As Range, CLinkRg As Range
Dim AllEntRg As Range, EntityOnlyRg As Range
Dim TypeRg As Range, GLRg As Range, OpsRg As Range
Dim FRow As Long, lRow As Long
Dim InlandRg As Range
Dim ATLNrg As Range, ATLErg As Range, ATLSrg As Range
Dim CCCodeRow As Long, CCCodeCol As Long
Sub Maverick()
starttime = Now()
Application.ScreenUpdating = False
Set Wb1 = Workbooks("SubModel Forecast_Other Admin v4.xlsm")
Set Wb2 = Workbooks("Feb15 PNL.xlsx")
Set Wk4 = Wb1.Sheets("ASSUMPTIONS")
Set Wk5 = Wb1.Sheets("Validation")
Set Wk7 = Wb1.Sheets("GL Mapping")
Set Wk1 = Wb2.Sheets("det")
With Wb1
With Wk5
Dim CCCol As Long, fRowCC As Long, lRowCC As Long
CCCol = Wk5.Cells.Find("Cost Center", lookat:=xlWhole).Column
fRowCC = Wk5.Cells.Find("Cost Center", lookat:=xlWhole).Offset(1, 0).Row
lRowCC = Wk5.Cells.Find("Cost Center", lookat:=xlWhole).End(xlDown).Row
Set CCRg = Wk5.Range(Wk5.Cells(fRowCC, CCCol), Wk5.Cells(lRowCC, CCCol))
Set CLinkRg = Wk5.Range(Wk5.Cells(fRowCC, CCCol).Offset(0, -1), Wk5.Cells(lRowCC, CCCol).Offset(0, -1))
End With
With Wk7
Dim MapGLCol As Long, MapfRow As Long, MaplRow As Long
MapGLCol = Wk7
{Auto - Sentra/Van Fleet ($) ; Other ($)} to be able to fit the code with its public functions. They repeat the same functionality expect the range input at the end of calling the functions changes. I'm assuming dictionaries will be a good starting point but I'm having difficulty comprehending its functionality.```
Dim Wb1 As Workbook, Wb2 As Workbook
Dim Wk4 As Worksheet, Wk5 As Worksheet, Wk7 As Worksheet
Dim Wk1 As Worksheet
Dim CCRg As Range, CLinkRg As Range
Dim AllEntRg As Range, EntityOnlyRg As Range
Dim TypeRg As Range, GLRg As Range, OpsRg As Range
Dim FRow As Long, lRow As Long
Dim InlandRg As Range
Dim ATLNrg As Range, ATLErg As Range, ATLSrg As Range
Dim CCCodeRow As Long, CCCodeCol As Long
Sub Maverick()
starttime = Now()
Application.ScreenUpdating = False
Set Wb1 = Workbooks("SubModel Forecast_Other Admin v4.xlsm")
Set Wb2 = Workbooks("Feb15 PNL.xlsx")
Set Wk4 = Wb1.Sheets("ASSUMPTIONS")
Set Wk5 = Wb1.Sheets("Validation")
Set Wk7 = Wb1.Sheets("GL Mapping")
Set Wk1 = Wb2.Sheets("det")
With Wb1
With Wk5
Dim CCCol As Long, fRowCC As Long, lRowCC As Long
CCCol = Wk5.Cells.Find("Cost Center", lookat:=xlWhole).Column
fRowCC = Wk5.Cells.Find("Cost Center", lookat:=xlWhole).Offset(1, 0).Row
lRowCC = Wk5.Cells.Find("Cost Center", lookat:=xlWhole).End(xlDown).Row
Set CCRg = Wk5.Range(Wk5.Cells(fRowCC, CCCol), Wk5.Cells(lRowCC, CCCol))
Set CLinkRg = Wk5.Range(Wk5.Cells(fRowCC, CCCol).Offset(0, -1), Wk5.Cells(lRowCC, CCCol).Offset(0, -1))
End With
With Wk7
Dim MapGLCol As Long, MapfRow As Long, MaplRow As Long
MapGLCol = Wk7
Solution
I like to start off by saying that your formatting is pretty good. Well done. But there are a few things to simplify your code and make it easier to understand. From there it will be easier to manage and even improve the performance.
Also, the use of functions to solve smaller problems is good.
Functions
Your comments about 'public functions' are a little vague. Functions are used to improve the maintainability of code and ideally allow you to reuse the functionality in other places. It doesn't add any performance (if anything calling a function decreases performance).
So for instance a common function I use is to find the last used row in a worksheet. It serves a specific purpose, is only 5-10 lines long, but I use it all over the place when I'm coding. The cool thing is that I can replace the way it works internally without affecting any of the consumers of that function.
The use of 'Public' (and also the alternative, Private) is to control who can access a function, variable, property. In programming this is called Scope. Basically when you make something Public you are telling other code that this function is for public use.
As I mentioned above, you should separate out logical chunks of code into separate functions. Some of these functions may not be suitable for use outside of their module. In this case they can be declared Private and they won't be visible outside that module.
Use Variables for repeated code references
you have a huge number of
Use of With Blocks
It appears you are not understanding how
Code Duplication
There is a huge amount of duplication in your code. Every time you use copy and paste you need to ask whether you are doing the right thing. Quite often you can see a pattern in what you are doing and can create a separate method (
Take for example the
```
' I have tried my best to factor out this method but I quite likely missed something,
' so carefully review this to make sure that nothing has been missed.
Private Function ProcessStuff(ByVal processRange As Range, ByVal currentRow As Range)
' Using With block to simplify calls to Sum().
With Application.WorksheetFunction
' The following two variables should be renamed to match the semantics of the data they represent.
Dim entryType As Range
entryType = currentRow.End(xlUp)
Dim outputCell As Range
Set outputCell = Wk4.Cells(currentRow.Row, 4)
' Using a select case statement instead of nested If hierarchy.
Select Case entryType
Case "Tie-Out To Actuals"
outputCell = .Sum(processRange)
Case "Entity Level Assumptions"
outputCell = .Sum(EntityGLRg(AllEntRg, processRange))
Case "Inland Empire"
outputCell = SinglePMRanges(entryType, processRange) _
+ .Sum(MultipleCostCenterGLRange(DefMultiCCPMRange(AllEntRg, "cahied"), DefMultiCCPMRange(AllEntRg, "cahrvr"), processRange))
Case "Atlanta East"
outputCell = SinglePMRanges(entryType, processRange) _
+ .Sum(MultipleCostCenterGLRange(DefMultiCCPMRange(AllEntRg, "atlse"), DefMultiCCPMRange(AllEntRg, "atle"), processRange))
Case "Atlanta North"
outputCell = SinglePMRanges(entryType, processRange) _
+ .Sum(MultipleCostCenterGLRange(DefMultiCCPMRange(AllEntRg, "atlnw"), DefMultiCCPMRange(AllEntRg, "atln"), processRange))
Case "Atlanta South"
outputCell = SinglePMRanges(entryType, processRange) _
+ .Sum(MultipleCostCenterGLRange(DefMult
Also, the use of functions to solve smaller problems is good.
Functions
Your comments about 'public functions' are a little vague. Functions are used to improve the maintainability of code and ideally allow you to reuse the functionality in other places. It doesn't add any performance (if anything calling a function decreases performance).
So for instance a common function I use is to find the last used row in a worksheet. It serves a specific purpose, is only 5-10 lines long, but I use it all over the place when I'm coding. The cool thing is that I can replace the way it works internally without affecting any of the consumers of that function.
The use of 'Public' (and also the alternative, Private) is to control who can access a function, variable, property. In programming this is called Scope. Basically when you make something Public you are telling other code that this function is for public use.
As I mentioned above, you should separate out logical chunks of code into separate functions. Some of these functions may not be suitable for use outside of their module. In this case they can be declared Private and they won't be visible outside that module.
Use Variables for repeated code references
you have a huge number of
r.End(xlUp) references in your code. I suggest you figure out what this range represents and use a variable to name it appropriately. Similarly with Wk4.Cells(r.Row, 4). I have actually pulled these into a method which you will see below.Use of With Blocks
It appears you are not understanding how
With blocks work. The reason I'm saying this is because I commented out all of the beginning and ending With statements and the code still compiles. The purpose of these blocks is to reduce repetition of an object reference inside the containing code. So for instance instead of writing a lot of statements like this Application.WorksheetFunction.Sum( ... ) You can replace them with this:With Application.WorksheetFunction
' Notice that we can leave out the Application.WorksheetFunction
' part and just add a leading '.'
.Sum( ... )
End WithCode Duplication
There is a huge amount of duplication in your code. Every time you use copy and paste you need to ask whether you are doing the right thing. Quite often you can see a pattern in what you are doing and can create a separate method (
Function or Sub) that can be created to solve that particular problem. The things that are changing between uses of that code become your arguments/parameters.Take for example the
Select Case statement inside the main loop of Maverick(). Each case has very similar code inside of it. The only things that appear to be changing each time is the range that is used. So you could create a new method which accepts a range argument and a reference to the output workbook (WK4 in this case) and performs the processing. Somthing similar to the following:```
' I have tried my best to factor out this method but I quite likely missed something,
' so carefully review this to make sure that nothing has been missed.
Private Function ProcessStuff(ByVal processRange As Range, ByVal currentRow As Range)
' Using With block to simplify calls to Sum().
With Application.WorksheetFunction
' The following two variables should be renamed to match the semantics of the data they represent.
Dim entryType As Range
entryType = currentRow.End(xlUp)
Dim outputCell As Range
Set outputCell = Wk4.Cells(currentRow.Row, 4)
' Using a select case statement instead of nested If hierarchy.
Select Case entryType
Case "Tie-Out To Actuals"
outputCell = .Sum(processRange)
Case "Entity Level Assumptions"
outputCell = .Sum(EntityGLRg(AllEntRg, processRange))
Case "Inland Empire"
outputCell = SinglePMRanges(entryType, processRange) _
+ .Sum(MultipleCostCenterGLRange(DefMultiCCPMRange(AllEntRg, "cahied"), DefMultiCCPMRange(AllEntRg, "cahrvr"), processRange))
Case "Atlanta East"
outputCell = SinglePMRanges(entryType, processRange) _
+ .Sum(MultipleCostCenterGLRange(DefMultiCCPMRange(AllEntRg, "atlse"), DefMultiCCPMRange(AllEntRg, "atle"), processRange))
Case "Atlanta North"
outputCell = SinglePMRanges(entryType, processRange) _
+ .Sum(MultipleCostCenterGLRange(DefMultiCCPMRange(AllEntRg, "atlnw"), DefMultiCCPMRange(AllEntRg, "atln"), processRange))
Case "Atlanta South"
outputCell = SinglePMRanges(entryType, processRange) _
+ .Sum(MultipleCostCenterGLRange(DefMult
Code Snippets
With Application.WorksheetFunction
' Notice that we can leave out the Application.WorksheetFunction
' part and just add a leading '.'
.Sum( ... )
End With' I have tried my best to factor out this method but I quite likely missed something,
' so carefully review this to make sure that nothing has been missed.
Private Function ProcessStuff(ByVal processRange As Range, ByVal currentRow As Range)
' Using With block to simplify calls to Sum().
With Application.WorksheetFunction
' The following two variables should be renamed to match the semantics of the data they represent.
Dim entryType As Range
entryType = currentRow.End(xlUp)
Dim outputCell As Range
Set outputCell = Wk4.Cells(currentRow.Row, 4)
' Using a select case statement instead of nested If hierarchy.
Select Case entryType
Case "Tie-Out To Actuals"
outputCell = .Sum(processRange)
Case "Entity Level Assumptions"
outputCell = .Sum(EntityGLRg(AllEntRg, processRange))
Case "Inland Empire"
outputCell = SinglePMRanges(entryType, processRange) _
+ .Sum(MultipleCostCenterGLRange(DefMultiCCPMRange(AllEntRg, "cahied"), DefMultiCCPMRange(AllEntRg, "cahrvr"), processRange))
Case "Atlanta East"
outputCell = SinglePMRanges(entryType, processRange) _
+ .Sum(MultipleCostCenterGLRange(DefMultiCCPMRange(AllEntRg, "atlse"), DefMultiCCPMRange(AllEntRg, "atle"), processRange))
Case "Atlanta North"
outputCell = SinglePMRanges(entryType, processRange) _
+ .Sum(MultipleCostCenterGLRange(DefMultiCCPMRange(AllEntRg, "atlnw"), DefMultiCCPMRange(AllEntRg, "atln"), processRange))
Case "Atlanta South"
outputCell = SinglePMRanges(entryType, processRange) _
+ .Sum(MultipleCostCenterGLRange(DefMultiCCPMRange(AllEntRg, "atlsw"), DefMultiCCPMRange(AllEntRg, "atls"), processRange))
Case Else
outputCell = SinglePMRanges(entryType, processRange)
End Select
End With
End FunctionDim r As Range
Dim isItem As Boolean
For Each r In AssumptionRg
Select Case r
Case "Evictions ($)"
isItem = True
ProcessStuff EvictionRg, r
Case "Credit Verification Fees ($)"
isItem = True
ProcessStuff CreditFeesRg, r
Case "Legal Counsel Fees ($)"
isItem = True
ProcessStuff LegalCounselFeesRg, r
Case "Office - Prop Mgmt ($)"
isItem = True
ProcessStuff OfficeMgmtRg, r
Case "Office - Rent ($)"
isItem = True
ProcessStuff OfficeRentRg, r
Case "Office - Utilities ($)"
isItem = True
ProcessStuff OfficeUtilitiesRg, r
Case "Office - Other ($)"
isItem = True
ProcessStuff OfficeOtherRg, r
Case "Bad Debt ($)"
Wk4.Cells(r.Row, 4) = Application.WorksheetFunction.Sum(CategoryGLRange("Bad Debt ($)"))
End Select
Next rContext
StackExchange Code Review Q#87057, answer score: 5
Revisions (0)
No revisions yet.