First, kudos for teaching yourself VBA - I was once in your shoes and how I wish I could have had Code Review to help me back then!
Let's start with the function's signature:
Function defineRange(mySheet As Object, startRow As Long, firstColumn As Long, Optional endRow As Long, Optional secondColumn As Long) As Object
It's not clear whether the function's implicit public accessibility is intended or not. If it means to be accessible from outside the module it's declared in, then it should be explicitly
Public. Otherwise, it should be
Private - a private procedure/function/method can only be invoked from within the module it belongs to. I'm thinking the intent is for it to be
You probably noticed pretty much everything in VBA type libraries is
Worksheet.Name - best practice is to "blend in" and write code that adheres to the naming standard. Hence,
DefineRange would be better. I like how the name begins with a verb (as it should!), however "define" doesn't strike me as the best choice in this case. The function is getting, or rather acquiring a
Range object. The Excel object model already provides an API for this, so reinventing-the-wheel is appropriate here :)
The parameters are all implicitly passed by reference (
ByRef), which is an unfortunate default. Would this be expected/desired behavior?
Dim firstRow As Long
firstRow = 12
Dim lastRow as Long
lastRow = 0
Dim target As Range
Set target = defineRange(Sheet1, firstRow, 1, lastRow)
Debug.Print lastRow ' prints 12, not 0. is that expected?
By passing the parameters
ByVal, instead of passing a pointer ("reference") to the value, you essentially pass a copy of that value, and the calling code's variables can't get modified by the function. In VB.NET and many other languages, the implicit default is to pass arguments by value: it's somewhat rare that a parameter needs to be passed by reference.
mySheet is declared as an
Object, which means the function will happily take a
Range, or even
MyClass for this
mySheet parameter... and then the
mySheet.Range call will fail at run-time with error 438 "can't find property or method" (unless there's a method named
Range on that
MyClass object, of course). By declaring it
As Object, you made every member calls against that object late-bound, meaning the compiler can't help you, and will happily let you try to invoke
It's usually best to avoid deferring failures to run-time, and fail at compile-time instead, whenever possible. We do this with early binding, by declaring objects using a specialized interface we know we can work with - in this case,
Excel.Worksheet, or just
Worksheet. We can do this, because when VBA is hosted in Excel, the VBA project is guaranteed to have a reference to the
Excel type library. If we were hosted in Word or PowerPoint, we would have to explicitly add that reference (through tools/references), or work late-bound and keep it
The parameter names are meant to be pairs, but they're inconsistent:
- startRow, endRow
- firstColumn, secondColumn
"Start/End" was a better, clearer idea than "First/Second" - I'd go and rename the parameters to have
Kudos for declaring an explicit return type - all
Function procedures return something, whether you declare a return type or not. Then again, it would be better to return a
Range rather than an
So, that covers the function's signature :)
There are a number of possible bugs and edge cases that should be handled. The first thing any function should do, is validate its inputs.
VBA doesn't have unsigned integer types, so a
Long could very well be a negative number, or it could be zero -- but Excel's object model isn't going to like you trying to get the cell at row
0 and column
There are several ways to deal with this. The simplest is to use
Debug.Assert at the very top of the function, i.e. halt program execution if assumptions aren't validated:
'Debug.Assert TypeOf mySheet Is Excel.Worksheet
Debug.Assert startRow > 0
Debug.Assert firstColumn > 0
Another way is to have a guard clause, again at the very top of the function, that explicitly throws an error given invalid arguments:
If startRow > 0 Then Err.Raise 5, "defineRange", "Argument 'startRow' must be greater than zero."
If firstColumn > 0 Then Err.Raise 5, "defineRange", "Argument 'firstColumn' must be greater than zero."
One advantage of using guard clauses, is that if a function is given arguments it cannot possibly work with, then we fail early. This makes it easier to debug if something goes wrong later: instead of dealing with a cryptic and rather useless "Method 'Range' of class 'Worksheet' failed" error, the code that tried to invoke our function now knows exactly what went wrong, and how to fix it.
The return value assignment will fail if
mySheet isn't the
mySheet.Range(Cells(startRow, firstColumn).Address, Cells(endRow, secondColumn).Address)
That's because the unqualified
Cells calls are context-dependent.
If the function is written in a worksheet module's code-behind, then it's implicitly
If the function is written anywhere else, then it's implicitly
[_Global].Cells, which ultimately resolves to
ActiveSheet.Cells. That isn't a problem if
mySheet is active, but then if it isn't...
That's guaranteed to throw the dreaded run-time error 1004 "application-defined error", because only Schrödinger's Range can belong to two worksheets at the same time.
You can fix this with a
Set defineRange = .Range(.Cells(startRow, firstColumn).Address, .Cells(endRow, secondColumn).Address)
. dereferencing operators qualifying the
Cells member calls with the object reference held by the
With block. It's equivalent to this:
Set defineRange = mySheet.Range(mySheet.Cells(startRow, firstColumn).Address, mySheet.Cells(endRow, secondColumn).Address)
That said, you don't need to work off the
Range is more than happy to work with the
Range references returned by
Set defineRange = mySheet.Range(mySheet.Cells(startRow, firstColumn), mySheet.Cells(endRow, secondColumn))
While that works, I find it quite a mouthful, and as shown above, it has too many reasons to fail to my taste - I'd probably split it up:
Dim startCell As Range
Set startCell = mySheet.Cells(startRow, firstColumn)
Dim endCell As Range
Set endCell = mySheet.Cells(endRow, secondColumn)
Making the return assignment rather trivial:
Set defineRange = mySheet.Range(startCell, endCell)