patternModerate
Extract values within date and zone ranges
Viewed 0 times
zoneanddatewithinrangesextractvalues
Problem
In order to automate a manual task I created the following code. As I've never written a program to this size, I'm curious if my use of variables, as well as, solutions are the most optimal way for handling.
In this program, the user will choose a Year, Month, and Zone. These choices are used to narrow down Month within Year, Calendar Weeks within Month, and States within Zones. These narrowed ranges will then extract the data within those parameters and put them in the main report.
All-in-all, looking to find if there is a better use of variables, as well as, to know if these are the best methods for narrowing the ranges.
```
Sub TransactionCT_Reference()
Dim wb As Workbook, wb2 As Workbook
Dim ws As Worksheet, ws2 As Worksheet
Dim yRow As Variant
Dim frstMrow As Long, scndMrow As Long, _
frstYrow As Long, scndYrow As Long, _
frstZrow As Long, scndZrow As Long, _
frstSrow As Long, scndSrow As Long, _
frstCWrow As Long, scndCWrow As Long, cwCount As Long, _
unkRow As Long, mCount As Long
Dim frstBZday As Double, frstCLday As Double, scndBZday As Double, _
scndCLday As Double
Dim frstMname As String, scndMname As String, _
frstZname As String, scndZname As String, _
cwName1 As String, cwName2 As String, drName As String, unk As String
Dim mRng As Range, yRng1 As Range, yRng2 As Range, _
zRng1 As Range, zRng2 As Range, sRng As Range, _
cell As Range, cell2 As Range, xlsRng As Range, _
sRng1 As Range, sRng2 As Range, sRng3 As Range, _
dRng0 As Range, dRng13 As Range, dRng14 As Range, _
dRng15 As Range, dRng16 As Range, dRng17 As Range, _
dRng18 As Range, dRng19 As Range, dRng22 As Range, _
dRng23 As Range, ws2Rng As Range
' Change to MASTER_Choices Cycle Time
Set wb = Workbooks("MASTER_Choices Cycle Time - In Work VBA Changes.xlsm")
' Change to "03 - Data"
Set ws = wb.Worksheets("03 - Data 2")
ws.Activate
'Set Year value
yRow = ws.Cells(lastrow + 2, 5)
' Set Month value
In this program, the user will choose a Year, Month, and Zone. These choices are used to narrow down Month within Year, Calendar Weeks within Month, and States within Zones. These narrowed ranges will then extract the data within those parameters and put them in the main report.
All-in-all, looking to find if there is a better use of variables, as well as, to know if these are the best methods for narrowing the ranges.
```
Sub TransactionCT_Reference()
Dim wb As Workbook, wb2 As Workbook
Dim ws As Worksheet, ws2 As Worksheet
Dim yRow As Variant
Dim frstMrow As Long, scndMrow As Long, _
frstYrow As Long, scndYrow As Long, _
frstZrow As Long, scndZrow As Long, _
frstSrow As Long, scndSrow As Long, _
frstCWrow As Long, scndCWrow As Long, cwCount As Long, _
unkRow As Long, mCount As Long
Dim frstBZday As Double, frstCLday As Double, scndBZday As Double, _
scndCLday As Double
Dim frstMname As String, scndMname As String, _
frstZname As String, scndZname As String, _
cwName1 As String, cwName2 As String, drName As String, unk As String
Dim mRng As Range, yRng1 As Range, yRng2 As Range, _
zRng1 As Range, zRng2 As Range, sRng As Range, _
cell As Range, cell2 As Range, xlsRng As Range, _
sRng1 As Range, sRng2 As Range, sRng3 As Range, _
dRng0 As Range, dRng13 As Range, dRng14 As Range, _
dRng15 As Range, dRng16 As Range, dRng17 As Range, _
dRng18 As Range, dRng19 As Range, dRng22 As Range, _
dRng23 As Range, ws2Rng As Range
' Change to MASTER_Choices Cycle Time
Set wb = Workbooks("MASTER_Choices Cycle Time - In Work VBA Changes.xlsm")
' Change to "03 - Data"
Set ws = wb.Worksheets("03 - Data 2")
ws.Activate
'Set Year value
yRow = ws.Cells(lastrow + 2, 5)
' Set Month value
Solution
Boy is there a lot to talk about here.
First things first, if
Now, I'm going to point out all the good things you're already doing:
-
Explicitly declaring your variables as a type. This helps prevent all sorts of unintentional errors because the VBE will tell you when you're doing something you obviously did not intend to re: data types.
-
Using
-
Decent use of comments to document/signpost the logic of your code. These could be a lot better (which I will elaborate on) but they're enough to make your current code quite easy to follow and understand.
-
Actually using the VBA object model. A lot of beginner code is littered with multiple repetitions of
-
Determining values dynamically rather than hard-coding them. Having to hard-code anything is a huge source of bugs, headaches, and lost developer time for constant revisions. Anything that can be determined dynamically generally should be so thumbs up for that.
-
Refactoring. You could (and should) be doing an awful lot more of this. Taking a small thing (finding the last row) and moving it into a separate procedure is the foundation of all good code. So well done there.
In short: you actually have the foundation of a lot of the concepts you need to be a great developer. Now that that's out of the way:
Naming
Good, informative naming is one of the most, if not the most important part of developing software. Coding is roughly 80% reading code, and only 20% writing it. And that only gets even more tilted towards reading as your code gets bigger and more complicated. As such, in almost all circumstances, the most important part of your code is how easily somebody else can understand what it's doing.
Variable names should be Descriptive, Concise and, above all, Unambiguous. And they should sound like what they are.
NONE of your variables have good names. Barely any of them have even acceptable names.
You see this?
How about this?
Perfectly incomprehensible. If you thought one of the outputs of your code was wrong, where would you even begin trying to figure out what everything was doing?
My advice: Until you have the benefit of experience, err heavily on the side of more-verbose names. Even if it means writing variables called
Some examples:
etc. Don't abbreviate. Don't simplify. Just call them what they are
Don't use numbered variables. There are, occasionally, good reasons to do so. But they are rare. Off the top of your head, what piece of data does
Your ranges are a mess. They're crying out for some descriptive names.
What's easier to follow?
or
Notice how I took that line completely out of context, and yet it's pretty obvious what all the variables are, what they represent and what's going on.
Added bonus: Descriptive names are very hard to mis-type. Whereas, if your finger slips and you accidentally wrote
Refactoring
The next part of writing great code is s
First things first, if
Option Explicit is not at the top of every code module, go to tools --> Options --> Require Variable declaration. This will automatically insert it from now on (I honestly have no idea why it was optional in the first place).Now, I'm going to point out all the good things you're already doing:
-
Explicitly declaring your variables as a type. This helps prevent all sorts of unintentional errors because the VBE will tell you when you're doing something you obviously did not intend to re: data types.
-
Using
_ to organise your code in a more easily-readable way.-
Decent use of comments to document/signpost the logic of your code. These could be a lot better (which I will elaborate on) but they're enough to make your current code quite easy to follow and understand.
-
Actually using the VBA object model. A lot of beginner code is littered with multiple repetitions of
sheets("sheet name") etc. Here, you create your worksheet/book objects and then reference those, which means if, for instance, the name of your workbook changed, you'd only have to change it in one place in your code.-
Determining values dynamically rather than hard-coding them. Having to hard-code anything is a huge source of bugs, headaches, and lost developer time for constant revisions. Anything that can be determined dynamically generally should be so thumbs up for that.
-
Refactoring. You could (and should) be doing an awful lot more of this. Taking a small thing (finding the last row) and moving it into a separate procedure is the foundation of all good code. So well done there.
In short: you actually have the foundation of a lot of the concepts you need to be a great developer. Now that that's out of the way:
Naming
Good, informative naming is one of the most, if not the most important part of developing software. Coding is roughly 80% reading code, and only 20% writing it. And that only gets even more tilted towards reading as your code gets bigger and more complicated. As such, in almost all circumstances, the most important part of your code is how easily somebody else can understand what it's doing.
Variable names should be Descriptive, Concise and, above all, Unambiguous. And they should sound like what they are.
NONE of your variables have good names. Barely any of them have even acceptable names.
You see this?
Set dRng13 = ws.Cells(lastrow + j + 1 + 1 + mCount * k, 1)How about this?
scndBZday = sRng2.Cells(1, 2)Perfectly incomprehensible. If you thought one of the outputs of your code was wrong, where would you even begin trying to figure out what everything was doing?
My advice: Until you have the benefit of experience, err heavily on the side of more-verbose names. Even if it means writing variables called
thingWhichRepresentsOutputOfThatThing or procedures called FunctionToDoThisThingBasedOnThatAssumption. Sure, it's a pain to write out (pro-tip, use Ctrl+space to auto-complete any declared variable names, procedures, methods etc.), and it looks messy but believe me, it'll save you (or whoever else has to maintain your code) so much trouble down the road. Some examples:
wb1, wb2 are not good variable names. How am I suppsed to remember which is which? Especially if there's more than just 2 to keep track of. wbMasterChoices and wbDataOutput are still not great, but they're much more descriptive. Since you have more knowledge about what they actually are, you can probably think of some even better names.frstMRow, frstYRow, frstDRowetc. Don't abbreviate. Don't simplify. Just call them what they are
firstMonthRow, firstYearRow, firstDayRow. it's only a few extra characters but it's much simpler and easier on the brain.Don't use numbered variables. There are, occasionally, good reasons to do so. But they are rare. Off the top of your head, what piece of data does
drng19 represent? How about ws2rng? Or zrng2? If you can't tell what a variable is representing just by reading its name then it's not named well enough.Your ranges are a mess. They're crying out for some descriptive names.
yrng1, yrng2 to yearDataTable, yearIndexColumnmRng to monthDataTablemCount to monthRowCountWhat's easier to follow?
`mCount = Application.WorksheetFunction.CountIf(mRng, frstMname)`or
monthRowCount = Application.WorksheetFunction.CountIf(monthDataTable, firstMonthName)Notice how I took that line completely out of context, and yet it's pretty obvious what all the variables are, what they represent and what's going on.
Added bonus: Descriptive names are very hard to mis-type. Whereas, if your finger slips and you accidentally wrote
zrng when you meant to type srng, your program might give you the wrong output, and you'd spend forever trying to figure out what was going wrong, if you even realised in the first place.Refactoring
The next part of writing great code is s
Code Snippets
`mCount = Application.WorksheetFunction.CountIf(mRng, frstMname)`monthRowCount = Application.WorksheetFunction.CountIf(monthDataTable, firstMonthName)`'Find week data and transfer to MASTER sheet`Dim dataArray as variant
dataArray = array()
dataArray = GetWeekData(arg, arg, arg)
PrintWeekDataToSheet dataArray, wsSub BtnSort_Click()
'/====================================================================================================
'/ Author: Zak Armstrong
'/
'/ Description:
'/ For the active sheet, finds the data Table and sortKey columns using headers.
'/ Sorts clients based on payment frequency, then payment day, then Client Name.
'/ Colours rows depending on their payment frequency.
'/====================================================================================================
StoreApplicationSettings
DisableApplicationSettings
Dim ws_this As Worksheet
Set ws_this = ActiveSheet
Dim tableRange As Range
Set tableRange = GetTableRange(ws_this)
ValidateTableHeaders ws_this, tableRange
Dim paymentFrequencyColNum As Long, paymentDayColNum As Long, clientNameColNum As Long
FindColumnIndexes ws_this, tableRange, paymentFrequencyColNum, paymentDayColNum, clientNameColNum
SortTableRange ws_this, tableRange, paymentFrequencyColNum, paymentDayColNum, clientNameColNum
FormatTableRange ws_this, tableRange, paymentFrequencyColNum
RestoreApplicationSettings
End SubContext
StackExchange Code Review Q#114183, answer score: 11
Revisions (0)
No revisions yet.