Private Sub GetLastRow()
I like that it's Private
. Other than that... a Sub
doesn't "get" anything; a Sub
does something - this is a side-effecting procedure with the lipstick of a Function
. A pig with lipstick is still a pig though. Let's look at how it's implemented:
'Step 1: Declare Your Variables.
Dim LastRow As Long
'Step 2: Capture the last used row number.
LastRow = Cells(Rows.Count, 8).End(xlUp).Row
'Step 3: Select the next row down
Cells(LastRow, 8).Offset(1, 0).Select
These comments are noise. Comments that say what the code does are useless clutter, unless the code is absolutely unreadable... in which case the solution is to improve the code's readability. Good comments say why, not what. Bad comments should be removed.
There are a number of serious issues with that procedure. Cells
and Rows
both implicitly refer to the active worksheet, which means if the user happens to activate another sheet while the code is running, everything falls apart (I see you've addressed that elsewhere though) - it also falls apart if other code activates another sheet than the one that's expected to be active though. And given how your entire code [ab]uses Select
and Activate
, that's a real concern.
This would be my GetLastRow
:
Private Function GetLastRow(ByVal sheet As Worksheet, Optional ByVal fromColumn As Long = 1) As Long
GetLastRow = sheet.Cells(sheet.Rows.Count, fromColumn).End(xlUp).Row
End Function
No side effects. Returns ("gets") a value. Says what it does, does what it says: it returns the last row; a better name for a side-effecting procedure that selects the first empty row could be SelectFirstNonEmptyRowInColumnH
. Too specific? It's exactly what it does though. The problem isn't the specific name - the problem is what the procedure does.
The benefits of using non-side-effecting functions are numerous: first, I can call my function off any worksheet (not just the active one), make it look at any column, and I can call it 20 times in a row on 20 different sheets, not once will my Application.Selection
be affected.
It also makes code that reads exactly like what it does:
Call GetLastRow
Regardless of the Call
keyword being useless here (or anywhere else - it's been obsolete since the advent of the implicit procedure call syntax... over 20 years ago), reading the above line of code has a very different feel than reading this one:
sheet.Cells(GetLastRow(sheet, 8), 8).Offset(1).Select
More complicated? Ok then, let's split it up:
Const importantColumn As Long = 8
Dim lastRow As Long
lastRow = GetLastRow(sheet, importantColumn)
sheet.Cells(lastRow, importantColumn) _
.Offset(1) _
.Select
By not hiding .Select
operations behind a side-effecting procedure call [disguised as a function], we make it clear that we're tampering with the workbook's state.
There's only ever ONE reason to use .Select
in VBA code: when you want to set the current selection for the user.
Using .Select
for any other reason, is asking for your code to be as slow as Excel VBA code can get, not to mention extremely frail and bug-prone.
Working off Selection
is dangerous. In fact, your code can very easily blow up on the 2nd executable statement:
If Selection.Rows.Count > 1 Then
How? You assume Selection
is a Range
. But Selection
can be a Shape
. Selection
can be a Chart
, or a Series
. Selection
can be Nothing
. Each of these make execution jump to your ErrorHandler
subroutine, to display a cryptic error message in the debug/immediate pane, that doesn't tell anything about what's going on, where, or why:
Error number: 91 Object variable or With block variable not set.
Or
Error number: 438 Object doesn't support this property or method.
But programmers making assumptions (Selection
is a Range
?) are a dangerous thing: you'll likely never encounter these error messages, because when you test your macro yourself, Selection
will be a Range
. Things get more fun when it's an actual user running your code though. Users are particularly bad at wording useful bug reports - most of the time they sound like a downvoted Stack Overflow question that's accumulating "unclear what you're asking" close votes:
I clicked your macro button but nothing happens, it's not working.
Good luck figuring that one out!
Instead, you could separate responsibilities, increase the level of abstraction, and have smaller procedures that do one thing - and have few reasons to fail. Then you could include the procedure's name in your Debug.Print
statement, and at least you'd know which step blew up.
I'd review the actual crux of the code, but the [lack of] indentation makes my head spin, so instead I'll point you to Rubberduck and its Smart Indenter tool (disclaimer: I manage this open-source project; credits to @Comintern for most of the work on the Smart Indenter port), which can help you fix this.
Please read: How to avoid using Select in Excel VBA macros.