Problem with UDF designed to concatenate multiple matches in vlookup

[michaeltsmith93]

For the life of me, I can't figure out why this isn't working. It gives me a #VALUE error.

I'm using ActiveSheet because I'll put it on a number of different sheets, and I don't want to have to add a field in the function for that.

LookupRange is designed to find the last row with data in it on ActiveSheet.

My lookup values begin in B5 and extend indefinitely, and the desired matches are in Column O (15th column).

Function EmailConcat(LookupValue As String)


Application.Volatile


Dim i As Long
Dim Result As String
Dim LookupSheet As Worksheet
Dim LookupRange As Range


Set LookupSheet = Application.ActiveSheet


LookupRange = LookupSheet.Cells.Find(What:="*", SearchOrder:=xlRows, _
    SearchDirection:=xlPrevious, LookIn:=xlValues).Row


For i = 5 To LookupRange.Rows.Count
    If LookupSheet.Cells(i, 2) = LookupValue Then
    
    Result = Result & LookupSheet.Cells(i, 15) & "; "
    
    End If
Next i


EmailConcat = Left(Result, Len(Result) - 2)


End Function

 

[Yongle]

You are not being consistent

Variable LookUpRange is declared as a "range",
then created as if type "long" in FIND(..).Row,
then used (in the For loop) as if it were a "range"

Additionally LookUpRange will consist of ONE cell if determined by FIND in manner above, but you are treating it as a multi-row range with ...

For i = 5 to LookUpRange.Rows.Count

What do you want LookUpRange to be?
.Row returns the row number of the found cell

 

[michaeltsmith93]

A friend found a different way of going about it; see below.

I basically wanted LookupRange to be UBound, as in the code below. I think that I should have declared it as Long in the above.

Function EmailConcat(LookupValue As String)


    Application.Volatile


    Dim vals, rv, i As Long, sep As String


    If LookupValue <> "" Then
        With Application.ThisCell.Worksheet
            vals = .Range(.Range("B5"), .Cells(.Rows.Count, 2).End(xlUp))
            For i = 1 To UBound(vals, 1)
                If vals(i, 1) = LookupValue Then
                    rv = rv & sep & .Cells(4 + i, 15).Value
                    sep = "; "
                End If
            Next i
        End With
    End If
    EmailConcat = rv


End Function

 

[RoryA]

This part:

sep = "; "

shouldn't be inside your loop. There's no point setting the same value over and over again, especially in a volatile UDF.

Function EmailConcat(LookupValue As String)


    Application.Volatile
    Const sep as string = "; "

    Dim vals, rv, i As Long


    If LookupValue <> "" Then
        With Application.ThisCell.Worksheet
            vals = .Range(.Range("B5"), .Cells(.Rows.Count, 2).End(xlUp))
            For i = 1 To UBound(vals, 1)
                If vals(i, 1) = LookupValue Then
                    rv = rv & sep & .Cells(4 + i, 15).Value
                End If
            Next i
        End With
    End If
    If len(rv) <> 0 then EmailConcat = Mid$(rv, len(sep) + 1)

End Function

though it might be nicer to pass it as an optional argument in case you want to change it.

 

[Yongle]

thanks for posting your solution

 

[njimack]

Using ActiveSheet can be problematic if you're in a different sheet and the file is recalculated. I'd suggest using

Set ws = Sheets(Application.Caller.Parent.Name)

 

[michaeltsmith93]

Thanks for this, Rory. I had thought about that actually. I just figured it wasn't consuming a huge amount of processing power to set it over and over again.

I really appreciate you guys being patient with me--I've only just started learning VBA.

Could you please clarify what you mean by that last sentence?

Forgive my ignorance--is there any difference between these two?

Const sep as string = "; "

and a combination of

Dim sep as String

and

sep = "; "

 

[michaeltsmith93]

Hi Neil,

I meant to ask about that, too in my original post. In the solution posted above, I've used Application.ThisCell.Worksheet which accomplishes the same. Thank you for posting your idea--that could come in handy if I want to pull the name of the worksheet itself.

 

[RoryA]

Forgive my ignorance--is there any difference between these two?

Const sep as string = "; "

and a combination of

Dim sep as String

and

sep = "; "

In the former, sep is a constant (i.e. can't be changed) and so is allocated in memory once. In the latter it's a variable, and so can be changed (and is, repeatedly in your code).

As I mentioned at the end, my preference would be to actually make it an optional argument to the function (with a default value), so that you can change it in the cell formula if so desired without needing to amend any code.

 

[michaeltsmith93]

Okay, that makes perfect sense.

Ahh, I see what you mean. Good point. In this case, it's alright because I'm concatenating emails for Outlook which likes "; ", but I will keep that in mind.