4
\$\begingroup\$

This function works, but seems unnecessarily complicated. I would like to simplify but performance is vital as it is called many times.

The conditions are:

1) If either value is negative, then result is -1

2) If both values are zero, then result is zero

3) If NeedMax is true, result is largest value

4) If NeedMax is false, result is smallest non-zero value

CODE:

Public Function SelValue(ValOne As Long, ValTwo As Long, _
                    Optional NeedMax As Boolean = True) As Long

    If ValOne < 0 Or ValTwo < 0 Then
        SelValue = -1
    ElseIf ValOne = 0 And ValTwo = 0 Then
        SelValue = 0
    ElseIf NeedMax Then
        If ValOne > ValTwo _
            Then SelValue = ValOne _
            Else SelValue = ValTwo
    Else
        If ValOne = 0 Then
            SelValue = ValTwo
        ElseIf ValOne > ValTwo And ValTwo <> 0 Then
            SelValue = ValTwo
        Else
            SelValue = ValOne
        End If
    End If

End Function

Can anyone suggest a more direct, and hopefully faster, approach?

\$\endgroup\$
  • \$\begingroup\$ Are the parameters more likely to be positive or negative? It's called many times in what context? \$\endgroup\$ – Mathieu Guindon Dec 5 '18 at 19:49
  • 1
    \$\begingroup\$ @MathieuGuindon They should always be positive. The check for negatives is really an error check so that if SelValue is returned as -1 I can check for that. It is called to find an index number for a find result. \$\endgroup\$ – Rey Juna Dec 5 '18 at 19:51
  • 2
    \$\begingroup\$ That's a code smell. If legal values should be positive, then illegal values should throw an error (#5 "invalid procedure call or argument" seems sensible), and that way you avoid a systematic check for exceptional conditions. Seeing the function in its usage context could be helpful and valuable IMO. \$\endgroup\$ – Mathieu Guindon Dec 5 '18 at 19:55
  • \$\begingroup\$ There really should never be a negative value. Maybe I am being over cautious, but I always try to cover the bases. \$\endgroup\$ – Rey Juna Dec 5 '18 at 19:58
  • 2
    \$\begingroup\$ I've rolled back the last edit, since it incorporates feedback from an answer. Please don't invalidate answers with edits! =) \$\endgroup\$ – Mathieu Guindon Dec 5 '18 at 20:04
2
\$\begingroup\$

Try to use meaningful names. What's names (SelValue, ValOne,...) stands for? It don't give info on their purpose or usage.

Furthermore, you can simplify a lot your conditional branchments, using only this:

If ValOne < 0 Or ValTwo < 0 Then
    SelValue = -1
' remove this following condition to discard case 4
Else If ValOne = 0 Or ValTwo = 0 Then
    SelValue = ValOne + ValTwo  
ElseIf ValOne > ValTwo <> NeedMax Then  
    SelValue = ValTwo
Else
    SelValue = ValOne
End If

not fully tested, but should do the trick

Edit : fixed case (4), where NeedMax is false and one of values is 0.

\$\endgroup\$
  • \$\begingroup\$ Except when one value is negative this always returns the sum of the two values. Doesn't meet my conditions. \$\endgroup\$ – Rey Juna Dec 6 '18 at 0:52
  • \$\begingroup\$ @ReyJunaRey did you tested? If one value (Or both) is negative, it return -1... \$\endgroup\$ – Calak Dec 6 '18 at 1:00
  • \$\begingroup\$ Correct, if one (or both) is negative it will return -1. Otherwise it always returns the sum of the two values whereas I would like to either return the maximum or minimum non-zero. \$\endgroup\$ – Rey Juna Dec 6 '18 at 1:02
  • \$\begingroup\$ No, it return the sum only if at least one of value is zero. Test it please ;) \$\endgroup\$ – Calak Dec 6 '18 at 1:04
  • 1
    \$\begingroup\$ Fixed, it should work now. this post was just "phonecrafted", and especially the line after the comment wasn't tested. \$\endgroup\$ – Calak Dec 6 '18 at 1:24
1
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ SelValue = If(ValOne > ValTwo, ValOne, ValTwo). Using the VBA If() function in this context is safe because all components are values, not objects and not likely to throw error in themselves. \$\endgroup\$ – AJD Dec 5 '18 at 20:15
  • \$\begingroup\$ @AJD, I like that approach. You should post as an answer. \$\endgroup\$ – Rey Juna Dec 5 '18 at 20:18
  • \$\begingroup\$ @AJD that's IIf ;-) \$\endgroup\$ – Mathieu Guindon Dec 5 '18 at 20:18
  • 1
    \$\begingroup\$ @AJD IIf is a function, so it involves stack manipulation - i.e. copying memory, pushing the stack pointer, popping back, etc. That's a horrible idea if you're looking for raw performance. \$\endgroup\$ – Comintern Dec 5 '18 at 20:19
  • \$\begingroup\$ @MathieuGuindon: thanks - yes IIf(). If() works in .Net as well and is safer. \$\endgroup\$ – AJD Dec 5 '18 at 20:26
1
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ Oh well, there goes my draft! :) \$\endgroup\$ – Mathieu Guindon Dec 5 '18 at 20:20
  • \$\begingroup\$ @MathieuGuindon - Reading fail. Edited (although I'll stand by the rest of that bullet). \$\endgroup\$ – Comintern Dec 5 '18 at 20:23
  • 1
    \$\begingroup\$ Based on the comment conversation though, it appears -1 is actually an error flag; hence I'd move that first case last, and throw error 5 - eliminating a highly redundant if-check at the call site. \$\endgroup\$ – Mathieu Guindon Dec 5 '18 at 20:24
  • 1
    \$\begingroup\$ @MathieuGuindon That would make a good answer. ;-) \$\endgroup\$ – Comintern Dec 5 '18 at 20:24
  • \$\begingroup\$ Yeah, I like the Select Case approach - I was going to add it to my answer. \$\endgroup\$ – AJD Dec 5 '18 at 20:37
0
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ This doesn't seem to work right when NeedMax is False and only one of the values is 0. It returns 0 when it should return the non-zero number (Condition 4). \$\endgroup\$ – Rey Juna Dec 7 '18 at 17:57
  • \$\begingroup\$ I see. I've skipped that condition. Anyway, implementing that condtion functionnaly although feasible, is not practical regarding performance. For sake of brieviety denote A and B as the numbers ValOne and ValTwo. Denote P as NeedMax and Q as the Boolean test (A>0 AND B>0) then SelValue =B + (A>B) * (B-A) + Q*(1+P)*((B-A) - 2*(A>B)*(B-A)) \$\endgroup\$ – AbsoluteNaught Dec 8 '18 at 1:10
  • \$\begingroup\$ Hmm, seems more complex to understand than the answer and, though I'm not sure that I entered it correctly, when I try it with A=3, B=1, and P=False I get a result of 9. \$\endgroup\$ – Rey Juna Dec 8 '18 at 1:33
  • \$\begingroup\$ You're right as there's a typo (- operater instead of +). SelValue =B + (A>B) * (B-A) + Q*(1+P)*((B-A) + 2*(A>B)*(B-A)) . As you may see, this implicit boolean casting may not suit your sake after performance optimization. \$\endgroup\$ – AbsoluteNaught Dec 8 '18 at 12:05
-2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I did it again! If() instead of IIf() \$\endgroup\$ – AJD Dec 5 '18 at 20:39
  • 2
    \$\begingroup\$ SelValue = CVErr(xlErrValue) is only useful if the function is a UDF. I would absolutely Err.Raise 5 here and still return a Long. \$\endgroup\$ – Mathieu Guindon Dec 5 '18 at 20:39
  • \$\begingroup\$ Returning Variant to be able to return an Error instead of Raising it is bad advice. That's even more true when seen through the lens of performance, where returning Variant incurs overhead for type-coercion and IDispatch/IUnknown. \$\endgroup\$ – Vogel612 Dec 5 '18 at 20:47
  • \$\begingroup\$ @Vogel612: depends on the scope of the lens. Yes, in the scope of the single function there are performance and storage/space hits, but at the wider scale, performance and functionality is improved due to the improved logic flow ("is that -1 really -1 or is it some sort of coded error result?") An optimal system requires that some or all of the sub-systems are sub-optimal. \$\endgroup\$ – AJD Dec 5 '18 at 23:28

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.