8
\$\begingroup\$

code from suggestions in comments here. I will still write it like the first time, so you do not need to read the old question.

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 kth match and returns a ref.

Public Function LOOKUPK(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                                                                          'if you encounter errors remove the first >'<

  LOOKUPK = CVErr(2023)
  If count_num - 1 <> Abs(Round(count_num - 1)) Then Exit Function                               ' no natural Number or 0 => exit function

  If lookup_vector.Areas.Count > 1 Then Set lookup_vector = lookup_vector.Areas(1)               'only first area to work with
  Set lookup_vector = Intersect(lookup_vector.Parent.UsedRange, lookup_vector)                   'skip ranges for speed

  If result_vector Is Nothing Then                                                               'no output => make one
    If lookup_vector.Rows.Count = 1 Xor lookup_vector.Columns.Count = 1 Then                     'only 1 row/columne + no output = inputrng = outputrng
      Set result_vector = lookup_vector

    ElseIf lookup_vector.Rows.Count > 2 And lookup_vector.Columns.Count = 2 Then                 '2 columns + >2 rows => split for vlookupk-mode
      Set result_vector = lookup_vector.Columns(2).Cells
      Set lookup_vector = lookup_vector.Columns(1).Cells

    ElseIf lookup_vector.Rows.Count = 2 And lookup_vector.Columns.Count > 2 Then                 '>2 columns + 2 rows => split for hlookupk-mode
      Set result_vector = lookup_vector.Rows(2).Cells
      Set lookup_vector = lookup_vector.Rows(1).Cells

    Else
      Exit Function                                                                              'not supported range-size => exit function

    End If
  Else                                                                                           'got output => check for everything
    If result_vector.Areas.Count > 1 Then Set result_vector = result_vector.Areas(1)             'only first area to work with
    Set result_vector = Intersect(result_vector.Parent.UsedRange, result_vector)                 'skip ranges for speed
    If Not (result_vector.Columns.Count = 1 Xor result_vector.Rows.Count = 1) Then Exit Function 'not supported range-size => exit function
  End If

  If Not (lookup_vector.Columns.Count = 1 Xor lookup_vector.Rows.Count = 1) Then Exit Function   'not supported range-size => exit function
  If Not case_sens Then lookup_value = LCase(lookup_value)                                       'case doesn't matter => make it *lower*

  Dim cell_count As Long                                                                         'for count in For Each
  Dim cell_value As Variant                                                                      'the value in For Each

  For Each cell_value In lookup_vector.Value
    cell_count = cell_count + 1
    If case_sens Then                                                                            'case does matter - check directly
      If cell_value Like lookup_value Then count_num = count_num - 1
    Else                                                                                         'case doesn#t matter - check lower only
      If LCase(cell_value) Like lookup_value Then count_num = count_num - 1
    End If
    If count_num = 0 Then Exit For                                                               'item found - skip future loops
  Next

  If count_num = 0 Then                                                                          'only return something if desired match was found
    If result_vector.Columns.Count = 1 Then                                                      'only 1 column => select row
      Set LOOKUPK = result_vector(cell_count, 1)
    Else                                                                                         'only 1 row => select column
      Set LOOKUPK = result_vector(1, cell_count)
    End If
  End If
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.
    • Does not need to be the same type (row/column) like the lookup_vector

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

Edit There was a big fail in the code I didn't noticed till now: It compared with = instead of Like. Really sorry. :(

\$\endgroup\$
  • \$\begingroup\$ I'm confused, there is an comment at the LOOKUPK = CVErr(2023) line in my module, but it is not here. Still I copied it in one step. Maybe i deleted it unintentionally... original comment: 'to leave the sub later directly as one line... but for now feel free act like there is nothing (cus there is already an answer including this fact) \$\endgroup\$ – Dirk Reichel Dec 20 '15 at 21:52
  • \$\begingroup\$ Because you should be using StrComp and I don't have a current cite for it, I'm only adding this as a comment, but you should use UCase if you want to compare case-insensitively. \$\endgroup\$ – Mark Hurd Dec 21 '15 at 3:11
  • \$\begingroup\$ @MarkHurd As said StrComp is out of the question as it doesn't act the way I need it. Using wildcards is something like the core of this UDF. However, is there an impact using LCase instead of UCase? \$\endgroup\$ – Dirk Reichel Dec 21 '15 at 3:22
  • \$\begingroup\$ It is minor, but this is code review :-) I'll try to find references as to using UCase instead of LCase later. \$\endgroup\$ – Mark Hurd Dec 21 '15 at 3:39
  • \$\begingroup\$ That would be really nice (I simply assumed that most letters will be lower case and this way it may be faster)... while I don't think that this will have a real impact, it still would count for the "good to know" corner and got my attention right now :) \$\endgroup\$ – Dirk Reichel Dec 21 '15 at 3:45
7
\$\begingroup\$
  • What's special about 2023?

     CVErr(2023)
    

    Introduce a constant so Mr(s). Maintainer knows what this error code means.

  • This is a lot of logic to parse for something so simple.

    If count_num - 1 <> Abs(Round(count_num - 1)) Then Exit Function                               ' no natural Number or 0 => exit function
    

    Extract a private method.

    If IsNegative(count_num) Then Exit Function
    

    Note that now the comment is superfluous and can be removed. Often, comments are a sign that we can express the code more clearly by creating a new abstraction.

  • You're accessing lookup_vector.Rows.Count and lookup_vector.Columns.Count more than often enough to extract a well named variable.

  • Instead of simply exiting when something is unsupported, consider returning an error. It's not always a good idea for UDFs, but can be a great idea. It depends on your use case.

  • Instead of lower casing strings for case insensitive comparison, use the StrComp function. It's theoretically faster. (Theoretically, I've not benchmarked it.)

\$\endgroup\$
  • \$\begingroup\$ CVErr(2023) will return #REF cus most wsfunctions return #VALUE and this way it is easier if catching different errors for different output. Will If IsNegative(count_num) Then Exit Function do it for 0 or 2.4? For set an error: why? it is already there. No need to to it a second time. And StrComp is no option cus it doesn't support placeholder. However, put the counts into a variable is something i should think over... Thanks for the recommendations till now :) \$\endgroup\$ – Dirk Reichel Dec 20 '15 at 21:43
  • 1
    \$\begingroup\$ @DirkReichel count_num is a Long, which is an integer type - an Int32 actually: it can never be 2.4, so if that line intends to exit if it's zero, all that needs to be there is If count_num = 0 Then Exit Function. Also, by placeholders I suppose you mean the Like syntax? Except, your code doesn't use Like at all, so StrComp is an option. \$\endgroup\$ – Mathieu Guindon Dec 20 '15 at 23:04
  • \$\begingroup\$ haha, lol true... I completely missed that... now I'm kinda embarrassed ^^; \$\endgroup\$ – Dirk Reichel Dec 20 '15 at 23:16
  • 1
    \$\begingroup\$ Sorry, having = instead of Like explains a lot. Taking this into account, the part with StrComp was completely correct. Oh and @MatsMug all I ever needed was a count_num > 1... by noticing this I didn't even realized you already mentioned that there is no Like at all... Right now I wish I could turn back time. Big sorry again. :( \$\endgroup\$ – Dirk Reichel Dec 21 '15 at 3:37
  • \$\begingroup\$ It's all good @DirkReichel. I was just confused for a moment. \$\endgroup\$ – RubberDuck Dec 21 '15 at 10:07
6
\$\begingroup\$

Forget about pretty alignments

Don't make comments line up vertically. I didn't even realise there were any comments initially because they're beyond the end of the scroll window and there's all that whitespace inbetween. Far from making your code more readible, it actually makes it less so.

How much development time did you spend making it all line up? Whenever you modify a line you'll have to re-position it. It's spending time to actively make your code less clear. Don't do it.

Just stick with conventional commenting: Either on a the preceeding line or immediately after the code. I also prefer "'/" for comments so I can distinguish between comments and commented-out code, like so:

  'Application.Volatile '/ If you encounter errors remove the first >'<

  LOOKUPK = CVErr(2023)

  '/ No natural Number or 0 => exit function
  If count_num - 1 <> Abs(Round(count_num - 1)) Then Exit Function 

  If lookup_vector.Areas.Count > 1 Then Set lookup_vector = lookup_vector.Areas(1) '/ Only first area to work with
  Set lookup_vector = Intersect(lookup_vector.Parent.UsedRange, lookup_vector) '/ Skip ranges for speed

  If result_vector Is Nothing Then '/ No output => make one
    If lookup_vector.Rows.Count = 1 Xor lookup_vector.Columns.Count = 1 Then '/ Only 1 row/columne + no output = inputrng = outputrng
      Set result_vector = lookup_vector

    ElseIf lookup_vector.Rows.Count > 2 And lookup_vector.Columns.Count = 2 Then '/ 2 columns + >2 rows => split for vlookupk-mode
\$\endgroup\$
  • \$\begingroup\$ FWIW tools like Smart Intender and Rubberduck 2.0 (still in development) automate the lining-up of comments at a specified column. But yeah if the maintainer doesn't use add-ins, it's annoying ;-) \$\endgroup\$ – Mathieu Guindon Dec 21 '15 at 0:35
  • 2
    \$\begingroup\$ Even then, I think leaving horizontal whitespace between code and comments hurts readability. \$\endgroup\$ – Kaz Dec 21 '15 at 0:42
  • \$\begingroup\$ I have an ws_change sub which does it automatically, so i have only to copy it to the "dev-cell" and then copy back... and with normal resolution it perfectly fits my screen... this way i never wasted a thought at it :P (it also lines up all my If, For and whatever, while there is normally no need for). I started it cus some comments like the '/ No output => make one I need to look twice that this is no part of the code, so i started to add some spaces and ended up like this... but if handing this code to someone, i will pull them back :) \$\endgroup\$ – Dirk Reichel Dec 21 '15 at 0:46

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.