5
\$\begingroup\$

TL;DR I'm self teaching VBA, learning about functions and passing arguments from a subroutine into the function and want to know if my very basic function to define a range is optimal and follows correct referencing/standards/practices.

I'm teaching myself VBA (mostly in MS Excel) as a starting point into programming in my spare time (what little of it I have) with the goal of a career change and am now learning how to utilise functions.

Below is a function I have written to define a range based on the variables passed as arguments. As you will see the worksheet, first row and first column are required arguments and the second row and second column are optional.

The function will assign the value of the first row (or column) to the second row (or column) if the optional argument is omitted.

Function defineRange(mySheet As Object, startRow As Long, firstColumn As Long, Optional endRow As Long, Optional secondColumn As Long) As Object

'will create a single row range
If endRow = 0 Then
    endRow = startRow
End If
'will create a single column range
If secondColumn = 0 Then
    secondColumn = firstColumn
End If

'Define the A1 reference for the range with cells.address parameter using arguments as the row and column index.
Set defineRange = mySheet.Range(Cells(startRow, firstColumn).Address, Cells(endRow, secondColumn).Address)

End Function

As an example I'd call it from a subroutine as follows:

Sub test()

Dim myRange As Object

Set myRange = defineRange(ThisWorkbook.Sheets(1), 5, 2, 10, 4)

myRange.Select

End Sub

The result is Range("B5:D10") is selected on sheet1 of the workbook.

Is the function code optimal? Have best practices been followed?

\$\endgroup\$
3
\$\begingroup\$

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 Public here.

You probably noticed pretty much everything in VBA type libraries is PascalCase. From Range to 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 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 Collection, some UserForm1, any 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 UserForm1 or 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 mySheet.Rnge.

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 As Object.

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 startColumn and endColumn, matching startRow and endRow.

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 Object.

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 -728.

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 ActiveSheet:

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 Me.Cells.

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...

Sheet1.Range(Sheet2.Cells(...), Sheet2.Cells(...))

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 With block:

With mySheet
    Set defineRange = .Range(.Cells(startRow, firstColumn).Address, .Cells(endRow, secondColumn).Address)
End With

Note the . 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 .Address string: Range is more than happy to work with the Range references returned by Cells(...):

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)
\$\endgroup\$
  • \$\begingroup\$ Fantastic! The functions signature section has cleared up all the bits I didn't quite understand. Learning is definately made a billion times easier with use of sites like this and SO. In the bugs/edge cases section I recieved a runtime error when .Address wad missing from the Range references - I think it was the dreaded Runtime error 1004. I prefer the code without .Address so I'll look into that further. I funnily enough had split the range reference as you mentioned inittialy but ended up cramming it all into the one line. Overall it seems I did okay! \$\endgroup\$ – Samuel Everson Mar 6 at 18:11
  • 1
    \$\begingroup\$ @SamuelEverson something isn't adding up, .Address shouldn't be needed. I've just now executed ?range(cells(1,1),cells(2,2)).address in the immediate pane and got $A$1:$B$2 as an output, as expected. The error would have to have been caused by something else. One thing I didn't mention, is the indentation: while generally consistent, Function...End Function blocks (i.e. procedure scopes) denote scopes and should prompt an indent level. Also, see if Rubberduck's code inspections can find more potential issues =) \$\endgroup\$ – Mathieu Guindon Mar 6 at 19:25
  • \$\begingroup\$ yes I agree. I probably won't be able to recreate the error I was getting (that seemed to be caused by the missing .Address) as I made a fair few changes throughout writing the code but if I figure it out I'll be sure to share the results. \$\endgroup\$ – Samuel Everson Mar 6 at 21:24
  • \$\begingroup\$ I did some debuging and I have put the above mentioned issue with .Address down to me incorrectly declaring something somewhere when I originally tested the code. I've made adjustments per your review and it all works as expected (except the description of the Err.Raise method is not appearing - the default description appears) but I'm looking into that to work out why. \$\endgroup\$ – Samuel Everson Mar 8 at 10:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.