6
\$\begingroup\$

My code finds the value that corresponds to two values in a table. It works fine, but I'd like to know what you might do differently.

Table sample:

sample table

Input:

=findval(3200,100)

Output:

4,6

Code:

Function findval(x As String, y As String)

Dim LastRow As Long
Dim LastCol As Integer
Dim x_rgn As Range
Dim y_rgn As Range
    With ActiveSheet
        LastRow = .Cells(.Rows.Count, "A").End(xlUp).Row
    End With
    With ActiveSheet
        LastCol = .Cells(1, .Columns.Count).End(xlToLeft).Column
    End With

Set x_rgn = Range(Cells(1, 1), Cells(1, LastCol))
Set y_rgn = Range(Cells(1, 1), Cells(LastRow, 1))

With x_rgn
    Set val_x = .Find(What:=x, _
                        After:=.Cells(.Cells.Count), _
                        LookIn:=xlFormulas, _
                        LookAt:=xlWhole, _
                        SearchOrder:=xlByRows, _
                        SearchDirection:=xlNext, _
                        MatchCase:=False)
    val_x = val_x.Address
    val_x = Range(val_x).Column
End With
With y_rgn
    Set val_y = .Find(What:=y, _
                        After:=.Cells(.Cells.Count), _
                        LookIn:=xlFormulas, _
                        LookAt:=xlWhole, _
                        SearchOrder:=xlByRows, _
                        SearchDirection:=xlNext, _
                        MatchCase:=False)
    val_y = val_y.Address
    val_y = Range(val_y).Row
End With
findval = Cells(val_y, val_x).Value
End Function
\$\endgroup\$
  • \$\begingroup\$ Hi, in S.O. @pnuts answered me: =INDEX(A1:G8,MATCH(J2,A:A,0),MATCH(I2,1:1,0)) \$\endgroup\$ – Fabrizio Oct 30 '15 at 14:55
  • \$\begingroup\$ Welcome to Code Review! I made some edits to your code, hoping to make it a bit easier to read for people. If there's anything you'd like focused on in a review of your code (maybe readability, or efficiency?) please edit it into the start of your question. \$\endgroup\$ – SuperBiasedMan Oct 30 '15 at 14:56
  • \$\begingroup\$ @SuperBiasedMan, thank for your editing (it's a italian problem we write/speak too much), ok for your tips (readability, efficiency) \$\endgroup\$ – Fabrizio Oct 30 '15 at 15:00
8
\$\begingroup\$

First step:

Write down your assumptions

These would be mine;

  • This data table has a header row in row 1 and a header column in column A.

  • The values in the headers could be any legal text.

  • The values in the headers are unique


What's the fastest way to find an exact match in a list?

My answer: A dictionary (or any other Linked-List object)

Dim rowList as Scripting.Dictionary, columnList as Scripting.Dictionary
    Set rowList = New Scripting.Dictionary
    Set columnList = New Scripting.Dictionary

    finalrow = Cells(Rows.Count, 1).End(xlUp).Row
    finalColumn = Cells(1, Columns.Count).End(xlToLeft).Column

Dim cellText As String

    For i = 1 To finalrow
        cellText = Cells(i, 1).Text
        rowList.Add cellText, i
    Next i

    For i = 1 To finalColumn
        cellText = Cells(1, i).Text
        columnList.Add cellText, i
    Next i

Now we have 2 dictionaries, where each row/column (i) is referenced by its cell value.

rowList.item("0") will return row "5", "100" returns row "6" and so on. This will work with any size of table and any possible values for the headers, so long as there are no repeated values.

Now you just take your 2 input values, access the associated items, and either they exist and you have your co-ordinates, or they don't exist, and you can tell your user.

For added speed, read your row/column into an array before iteratively adding them to your list.

Bonus: Aside from the initial time to create the lists, searches are effectively instant for any size of list and any number of searches.

\$\endgroup\$
  • \$\begingroup\$ good ideas, I think that I'll use this method in most applications. \$\endgroup\$ – Fabrizio Nov 30 '15 at 9:24
  • \$\begingroup\$ Just a note, to use dictionaries in VBA, you'll need to add a reference (in Tools --> References) to "Microsoft Scripting Runtime" \$\endgroup\$ – Kaz Nov 30 '15 at 9:28
  • \$\begingroup\$ Ya, to use yr tips I add Microsoft Scriptlet Library and Microsoft Scripting Runtime, and change the code to Dim rowList, columnList Set rowList = CreateObject("Scripting.Dictionary") Set columnList = CreateObject("Scripting.Dictionary") \$\endgroup\$ – Fabrizio Nov 30 '15 at 9:50
7
\$\begingroup\$

This is kind of abusive of the With syntax, but I do sympathize.

Set x_rgn = Range(Cells(1, 1), Cells(1, LastCol))
'...

With x_rgn
    Set val_x = .Find(What:=x, _
                        After:=.Cells(.Cells.Count), _
                        LookIn:=xlFormulas, _
                        LookAt:=xlWhole, _
                        SearchOrder:=xlByRows, _
                        SearchDirection:=xlNext, _
                        MatchCase:=False)
    val_x = val_x.Address
    val_x = Range(val_x).Column
End With

This is the only place you use the variable x_rgn, so you could just use the With block the way it was intended to be used.

With Range(Cells(1, 1), Cells(1, LastCol))
    Set val_x = .Find(What:=x, _
                        After:=.Cells(.Cells.Count), _
                        LookIn:=xlFormulas, _
                        LookAt:=xlWhole, _
                        SearchOrder:=xlByRows, _
                        SearchDirection:=xlNext, _
                        MatchCase:=False)
    val_x = val_x.Address
    val_x = Range(val_x).Column
End With

This will release the reference to the range as soon as you exit the With, and that's the purpose of it.

I will note though that x_rgn is wrong for a couple of reasons though.

  1. It should be x_rng as in "X Range".
  2. Don't use underscores in your names. Underscores have a special place in VBA. They represent Event Procedures and Interface Method implementations. Using the underscores elsewhere gets confusing for people. It's better to use camelCase for variable names.

Also watch calling Range(). This is the same as calling ActiveSheet.Range(). It's an implicit call, and we should always be as explicit as possible. Also, using the ActiveSheet can lead to subtle bugs. It's better to always know exactly which worksheet that you're working with.

The only other thing I notice is that you could definitely extract a procedure to reduce the duplication in the calls to .Find.

Otherwise, it's pretty good. You might want to consider how you would optimize this by determining which dimension was larger, and then searching it last, or by implementing a binary search, but these are optimizations that are unlikely to be faster than find and should be thoroughly bench-marked against your current code.

\$\endgroup\$
  • 1
    \$\begingroup\$ #RubberDuck, thank for your tips and suggestions \$\endgroup\$ – Fabrizio Nov 2 '15 at 10:41
  • \$\begingroup\$ You're welcome @Fabrizio! (If you want to "ping" someone, use the @ sign.) \$\endgroup\$ – RubberDuck Nov 2 '15 at 11:51

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.