Bdtyktl

This is a really funky way of formatting an If statement and is totally inconsistent with the rest of the procedure (and the way most people would format it):

    If ValOne > ValTwo _

        Then SelValue = ValOne _

        Else SelValue = ValTwo

I'd rearrange that as

    If ValOne > ValTwo Then

        SelValue = ValOne 

    Else 

        SelValue = ValTwo

    End If

First, the default value seems like it's overkill here. The function should be a Min function or a Max function, not both. It's not going to kill you to have two functions with clear arguments that describe what they are doing.

The naming is suspect too. SelValue doesn't sound like the name of a function - it sounds like a procedure or property. It invokes the idea of doing something, not returning a value. I'd rename it. Note that I can't even suggest a good name because of the first thing, because FindHigherOrLowerOfTwoNumbers() seems a little... weird.

As far as performance, that would be the last thing on my mind. This is only performing comparison operations between numbers, and that should be blazingly fast regardless of how many extra comparisons you've managed to sneak in there:

• You don't have to check explicitly to see if they're both zero. If they're equal and both happen to be zero, you'll get zero back anyway.


• If both of them are the same, that's the only check you need to do. Just pick one and return it.

VBA's If statements don't short circuit, so I'd structure this as a Select Case and filter away remaining cases on my way down.

Select Case True

    Case ValOne < 0 Or ValTwo < 0

        SelValue = -1

    Case ValOne = ValTwo

        SelValue = ValOne

    Case NeedMax

        If ValOne > ValTwo Then

            SelValue = ValOne

        Else

            SelValue = ValTwo

        End If

    Case ValOne < ValTwo

        SelValue = ValOne

    Case Else

        SelValue = ValTwo

End Select

I'm not sure the functional solution below may contribute to better performance, but it's a way of dealing with the three conditional expressions all at once.

The maximum of two numbers A and B can expressed functionally as:

(Abs(A + B) + Abs(A - B)) /2

and the minimum as:

(Abs(A + B) - Abs(A - B)) /2

Noting that the boolean value True is evaluated to -1 and False to 0, thus the expression:

- (1 + 2 * NeedMax)

is evaluated to 1 if NeedMax is True, and -1 otherwise.
Combining all the three preceding expressions yields the desired solution:

Public Function SelValue(ValOne As Long, ValTwo As Long, _

                    Optional NeedMax As Boolean = True) As Long

    SelValue = (Abs(ValOne + ValTwo) - (1 + 2 * NeedMax) * Abs(ValOne - ValTwo)) / 2

End Function

This returns the maximum of the two values if NeedMax is True and and the minimum otherwise.
Checking wether both values are zero is redundant; the max and min would have the same value; namely Zero.
Checking wether both values are negative is left (as in your comment) to the calling procedure.

This is Excel, so some Excel built-in perks can be used.

Return a Variant, not a defined type because you can then return an error quite easily. Yes this does apply to general VBA as well.

Return early from the function if you can to save further checks - although in this simple example it is not really necessary.

Every time I see an ElseIf I think that there is something in here that can be improved.

If you are looking to expand this later, then write it for such extension. The first example below just cleans up the code.

Public Function SelValue(ValOne As Long, ValTwo As Long, _

                    Optional NeedMax As Boolean = True) As Variant

    If ValOne < 0 Or ValTwo < 0 Then

        SelValue = CVErr(xlErrValue)

        Exit Function

    End If



    If ValOne = 0 And ValTwo = 0 Then

        SelValue = CLng(0)

        Exit Function

    End If



    If NeedMax Then

        SelValue = IIf(ValOne > ValTwo, CLng(ValOne), CLng(ValTwo)) ' But note comments about a minor performance hit 

        Exit Function

    End If



    If ValOne = 0 Then

        SelValue = CLng(ValTwo)

        Exit Function

    End If



    ' Now that we have cleaned out the special cases above. NeedMax is False

    If ValOne > ValTwo And ValTwo <> 0 Then

        SelValue = CLng(ValTwo)

    Else

        SelValue = CLng(ValOne)

    End If



End Function

The above code is not the final way I would leave it - personally I would refactor it again because I prefer only one exit/return from a function. But the above iteration highlights something important. It isolates the logic. And you don't handle the case where ValTwo = 0

Addendum: SelValue = CVErr(xlErrValue) is only useful if the function is a UDF. Raising error #5 (Err.Raise 5) here and still returning a Long is a valid approach for a non-UDF. Thanks to Mathieu Guindon.