Suggestion #1: Use Option Explicit
and/or use fewer global variables
Either MaxNumber, MaxPriority
are never explicitly defined, which is a very bad idea and is why you should use Option Explicit
, or they're Global Variables, which is redundant, because they're self-contained within this subroutine.
Suggestion #2: Don't work directly with worksheet ranges
Accessing a worksheet is slow. You don't notice because doing it once is almost-instant, but do it a thousand times and it starts adding up really fast.
Whenever I'm working with worksheet data, the first thing I do is read all of it into an Array:
Dim wsData As Variant
wsData = Array()
Dim dataTable As Range
Set dataTable = "" '/ Define your worksheet range
ReDim arrWsData(1 To Rows, 1 To columns) '/ Same size as the range
wsData = dataTable
Now you have a 2-dimensional array, with all your worksheet data laid out in exactly the same way as it was on the sheet. Excel can iterate over an Array orders of magnitude faster than over a range because it doesn't have to access the worksheet object every time.
You can print an array to a sheet by doing the same in reverse: Define your range to be the same size as your array. Range = Array.
Suggestion #3: Recursion
Assuming you put the range A1:F50
from WS1 in an array, you can now iterate over it like so:
Dim i As Long, j As Long, n As Long
Dim testValue As Variant
Dim printData As Variant
printData = Array()
ReDim printData(1 To lastRow, 1 To 5)
For j = 1 To lastRow
If wsData(j, 1) <= MaxPriority Then
n = n + 1
For i = 1 To 5
testValue = wsData(j, i + 1)
If testValue <> "(blank)" Then printData(j, i) = testValue
Next i
End If
Next j
you can now change the size of your data, or the progression of the sequence, or a whole host of other things by changing 1 or 2 numbers. And 20 lines of code just became 4.
You can now pick any range on WS3 and print your output data there.
I don't know why your output columns on WS3 Jump a column from 6 to 4, but you can always write specific parts of your array to specific parts of the output sheet if needed.
Suggestion #4: Meaningful Variable Names
What is WS1
? What is WS3
? I know they're worksheets, but which worksheets? They appear next to each other in multiple places in this subroutine. If you accidentally put a 1 instead of a 3 somewhere, you can't tell at a glance where you've gone wrong. Give your variables names
Even a simple wsInput
, wsOutput
would be so much better.
MaxNumber
Sounds like it should be, well, a number. Except it's a range. variables should say what they are. Even if that means writing VariableThatHoldsThisThing
, it'll save you so much time down the road in debugging and maintaining and less technical debt.
Here, I recommend something like dataTable<description>
(I don't know the context of your data, so you can think of a much more succinct name yourself).
On the other side, MaxPriority
is a good variable name. It holds a number which represents the maximum priority from some table of priority numbers.
Also NestedIfStatement
is a terrible procedure name. It tells me nothing about what the procedure does. Procedure names should be of the form <DoThing>
or Perfom<Action>On<Thing>
or any larger combination thereof. If you can't describe your procedure in a (reasonably) short name, then it's a good sign that it's trying to do too much, and should be split into several smaller procedures.