Please critique this UDF

[JenniferMurphy]

I need to do some calculations (sum, average, count) on a collection of lists of integers that represent product ratings. The lists are of variable length. I have written a little UDF that will do one of several calculations on each list. Here's some sample data:

And here's the UDF code:

Function ListCalc(pList, Optional pType As String = "Average")

Dim ListArray() As String
ListArray = Split(pList, " ")
Dim ListSum As Double
ListSum = 0
Dim ListCount As Long
ListCount = UBound(ListArray)
Dim I As Integer

For I = 0 To ListCount
    ListSum = ListSum + CDbl(ListArray(I))
Next I

Select Case UCase(pType)
    Case "SUM"
        ListCalc = ListSum
    Case "COUNT"
        ListCalc = ListCount + 1
    Case Else
        ListCalc = ListSum / (ListCount + 1)
End Select

End Function

Is there anything in this code that could be better? I never know, for example, if I am using the best datatypes.

Thanks

 

[eirikdaude]

I guess you are already aware that there are already existing functions in Excel which will do what your UDF does?

Strictly speaking using Longs instead of Integers for all integers in Excel VBA is better, because they will be converted to longs anyway.

If you just want to get the count of the range passed to the function, there is no reason to loop through your array.

You don't specify the datatype of the plist argument of the function, or what datatype the function will return. I like to make these things explicit whenever I can. Do you run into some trouble with, for instance, the Split function if you specify pList as a range?

All that said, there I'll reiterate my first point - there are already functions for doing what you describe here in Excel, and I'd be absolutely shocked if it was possible to get even close to the same efficiency from a UDF as from a native function.

- edit - On reading your post a bit closer, I see that you have several numbers in each cell, which makes the approach you're taking more understandable. That will teach me to just skim the source data. I think you may still be better off just using worksheet-functions (sum, average, counta, etc.) on the array you end up with though. I believe all of these will take an array as their argument

 

[footoo]

eirikdaude
I think you're mis-reading the OP's requirement which is to Sum/Count/Average the numbers in each cell in column F.
The UDF looks OK to me.

 

[FormR]

Hi, here's an alternative way of writing it.

Function ListCalc(pList, Optional pType As String = "Average") As Double
ListCalc = Evaluate(pType & "(" & Replace(pList, " ", ",") & ")")
End Function

Book4
	A	B
1	30 40 20	90
2	75	75
3	90 80 80 90 85	5
4	50 40 30 20 22	32.4
5	50 40 30 20 22	32.4
Sheet1

Cell Formulas
Range	Formula
B1	B1	=ListCalc(A1,"sum")
B2	B2	=ListCalc(A2,"Sum")
B3	B3	=ListCalc(A3,"count")
B4	B4	=ListCalc(A4)
B5	B5	=ListCalc(A5,"average")

 

[RoryA]

It's not particularly efficient to do all the calculations regardless of which one is actually needed, but other than that it looks ok to me.

 

[JenniferMurphy]

I guess you are already aware that there are already existing functions in Excel which will do what your UDF does?

. . .

All that said, there I'll reiterate my first point - there are already functions for doing what you describe here in Excel, and I'd be absolutely shocked if it was possible to get even close to the same efficiency from a UDF as from a native function.

- edit - On reading your post a bit closer, I see that you have several numbers in each cell, which makes the approach you're taking more understandable. That will teach me to just skim the source data. I think you may still be better off just using worksheet-functions (sum, average, counta, etc.) on the array you end up with though. I believe all of these will take an array as their argument

Maybe I'm missing something, but I don't think those workspace functions work on a cell with multiple values.

 

[JenniferMurphy]

Hi, here's an alternative way of writing it.

Function ListCalc(pList, Optional pType As String = "Average") As Double
ListCalc = Evaluate(pType & "(" & Replace(pList, " ", ",") & ")")
End Function

Wow! That works!! Thank you!!!

But what's really amazing is that the pType keywords that I chose just happen to be the correct Evaluate keywords.

Here's my new code. Now it only does the specified calculation and no looping!

Function ListCalc(pList, Optional pType As String = "Average")

Const ValidTypes As String = "AVERAGE SUM COUNT"

If InStr(1, ValidTypes, pType, 1) = 0 Then
  ListCalc = CVErr(xlErrValue)
  Exit Function
End If

ListCalc = Evaluate(pType & "(" & Replace(pList, " ", ",") & ")")

End Function

And here's some test data:

Just one problem. The InStr function conveniently returns a match if any portion of the pType string matches. Unfortunately, Evaluate requires the full type name. So in F10, passing "ave" as the type passes the InStr test, but fails the Evaluate call.

Is there an easy way to check if pType matches any of the valid types exactly?

Thanks

 

[JenniferMurphy]

It's not particularly efficient to do all the calculations regardless of which one is actually needed, but other than that it looks ok to me.

Agreed, but I was too lazy to add the extra code. But now I have a solution using Evaluate that eliminates that problem.

 

[JenniferMurphy]

I think I found a solution. I just needed to add a space after the last valid type ("COUNT ") and then add a space after pType in the InStr statement.

Here's that code, which seems to work:

Function ListCalc(pList, Optional pType As String = "Average")

Const ValidTypes As String = "AVERAGE SUM COUNT "

If InStr(1, ValidTypes, pType & " ", 1) = 0 Then
  ListCalc = CVErr(xlErrValue)
  Exit Function
End If

Select Case UCase(Left(pType, 1))
    Case "A": pType = "AVERAGE"
    Case "S": pType = "SUM"
    Case "C": pType = "COUNT"
End Select

ListCalc = Evaluate(pType & "(" & Replace(pList, " ", ",") & ")")

End Function

Thanks to everyone who contributed.