HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Function to find the kth match

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
findthematchfunctionkth

Problem

I've created a UDF some time ago and used it pretty often till now in different ways (but mainly to compare a "history" of data like having different filters at the same time). While there is no need to change anything at this state, I'd like get some suggestions of how to improve it without the loss of any functionality.

To make it short, it is a LOOKUP-function which goes for the xth match and returns a ref.

Normally there are comments, but I deleted them to test if it is still easy to get what it does. Also i like to have it in a way i don't need them.

```
Public Function VLOOKUPK(lookup_value As String, lookup_vector As Range, Optional result_vector As Range, _
Optional count_num As Long = 1, Optional case_sens As Boolean) As Variant
Application.Volatile

Dim i(0 To 1) As Long, j As Long, lookup_vector_values As Variant
VLOOKUPK = CVErr(2023)
If count_num - 1 <> Abs(Round(count_num - 1)) Or (i(0) * i(1)) = 1 Then Exit Function

Set lookup_vector = Intersect(lookup_vector.Parent.UsedRange, lookup_vector)
If lookup_vector.Areas.Count > 1 Then Set lookup_vector = lookup_vector.Areas(1)
i(0) = lookup_vector.Rows.Count
i(1) = lookup_vector.Columns.Count

If i(0) > 2 And i(1) > 2 Then Exit Function

If result_vector Is Nothing Then
If i(0) = 1 Or i(1) = 1 Then
Set result_vector = lookup_vector

ElseIf i(0) > 2 And i(1) = 2 Then
Set result_vector = lookup_vector.Columns(2)
Set lookup_vector = lookup_vector.Columns(1)

ElseIf i(0) = 2 And i(1) > 2 Then
Set result_vector = lookup_vector.Rows(2)
Set lookup_vector = lookup_vector.Rows(1)

Else
Exit Function

End If
Else
If result_vector.Columns.Count > 1 And result_vector.Rows.Count > 1 Then Exit Function
Set result_vector = Intersect(result_vector.Parent.UsedRange, result_vector)

End If

If lookup_vector.Columns.Count > 1 And lookup_vector.Rows.Count > 1 Then Exit Function

If Not case_sens The

Solution

Volatile

Application.Volatile


I'm walking down the code as I'm writing this, so I haven't really seen the code below that line. The Application.Volatile method call tells Excel to recalculate your function every time any cell anywhere on the worksheet is modified. The default behavior is to recalculate only when the input variables change.

That method call on the Application object tells me that you've perhaps tested it without at first, and concluded that it was required in order for the function to properly recalculate.

A comment explaining why Application.Volatile is needed here, might be judicious.

Naming notes...

I like the consistency you have with your naming style, it's nice to see. The style matches that of native Excel worksheet functions, like SUM:

SUMIF(range, criteria, [sum_range])

I think it's fine to expose that style to outside code (and/or Excel), but I don't see a reason for the internals to use any snake_case, convention is to use camelCase for locals (and usually, parameters too), and PascalCase for member names (procedures, functions, properties, ..).

Dim i(0 To 1) As Long, j As Long, lookup_vector_values As Variant


Hmm. 3 variables declared in the same instruction, of 3 different types and meanings. For readability's and maintainability's sake, a few points:

  • Declare arrays on their own line. As Long, j As Long is pretty much hiding the identifier.



  • If you're going to declare more than one variable in one instruction, do so but with variables of the same type.



  • Use meaningful identifier names: i is usually used in a For..Next loop, to count iterations. An array of Int32's isn't exactly trivially that. We don't know anything about that variable until we stumble on a usage of it, which hopefully isn't too far. The first usage is actually 2 lines below, which isn't too bad, but... let's face it, we all hate it when it's down to i and l and 1.



  • j is also typically used for iterating Foo..Next loops, usually when i is already taken. I'll have to scroll further down to see where the loop is. An interesting loop, I'll get back to it.



  • Declare variables as close as possible to their first usage.



I'd get rid of i, and rename lookup_vector to lookup_range or, as VLOOKUP has it, table_array (although, ...)

I said I was going to get back to the loop:

For i(j) = 1 To UBound(lookup_vector_values, j + 1)


Why make that operation a side-effect of iterating the loop? It's non-standard; people usually use a variable in a For loop, not an expression - one must read it carefully. If it can't be refactored, then it should be commented.

Code Snippets

Application.Volatile
Dim i(0 To 1) As Long, j As Long, lookup_vector_values As Variant
For i(j) = 1 To UBound(lookup_vector_values, j + 1)

Context

StackExchange Code Review Q#114425, answer score: 4

Revisions (0)

No revisions yet.