11
\$\begingroup\$

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 Then lookup_value = LCase(lookup_value)
  If lookup_vector.Columns.Count > 1 Then j = 1
  lookup_vector_values = lookup_vector.Value
  i(0) = 1: i(1) = 1

  For i(j) = 1 To UBound(lookup_vector_values, j + 1)
    If lookup_value Like IIf(case_sens, lookup_vector_values(i(0), i(1)), LCase(lookup_vector_values(i(0), i(1)))) Then
      If count_num = 1 Then
        Set VLOOKUPK = result_vector.Cells(i(0), i(1))
        Exit Function

      Else
        count_num = count_num - 1

      End If
    End If
  Next
End Function

How to use it:

VLOOKUPK(lookup_value,lookup_vector,[result_vector],[count_num],[case_sens])

  • lookup_value (required)

    • The value to search for.
    • lookup_value can be used with wildcards like *.
  • lookup_vector (required)

    • The Range to look in.
    • It needs to contain only one row or column if the result_vector is submitted.
    • If no result_vector is set and the lookup_vector contains only 1 row/column, the result_vector will be set to the lookup_vector.
    • If no result_vector is set, and the lookup_vector contains 2 columns while holding more then 2 rows, the first column will be set as lookup_vector while the second column will become the result_vector. (and vice versa)
      • Having a range of 2x2 will result in an error.
  • result_vector (optional)

    • The range where the value to output is located.
    • Needs to contain only one row or column.
  • count_num (optional)

    • Indicates the kth match to work with.
    • if not submitted, the first occurrence will be submitted.
  • case_sens (optional)

    • if set to TRUE the search will be case sensitive.
\$\endgroup\$
1
  • \$\begingroup\$ I would suggest to put the comments back in. Good use (or not) of comments is something that often gets reviewed, and their presence is not going to stop us reviewing your code for clarity either. \$\endgroup\$
    – Kaz
    Dec 19, 2015 at 13:01

1 Answer 1

4
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ For the Application.Volatile: While it always worked for me without, some ppl told me it does not (and I don't know how that could happen with this simple UDF). I took the names from LOOKUP cus it works with columns and rows so I will probably stay with it. But having (after few changes) only one row/column to check and output, I will change it to a For Each ... loop and then simply count them till the set match. Will also kill the i(). Thanks for now, will be back soon :) \$\endgroup\$ Dec 19, 2015 at 21:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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