Function for minimum non-zero with multiple additional conditions











up vote
2
down vote

favorite
1












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?










share|improve this question









New contributor




Rey Juna is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.




















  • Are the parameters more likely to be positive or negative? It's called many times in what context?
    – Mathieu Guindon
    2 hours ago








  • 1




    @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.
    – Rey Juna
    2 hours ago








  • 2




    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.
    – Mathieu Guindon
    2 hours ago










  • There really should never be a negative value. Maybe I am being over cautious, but I always try to cover the bases.
    – Rey Juna
    2 hours ago






  • 2




    I've rolled back the last edit, since it incorporates feedback from an answer. Please don't invalidate answers with edits! =)
    – Mathieu Guindon
    2 hours ago















up vote
2
down vote

favorite
1












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?










share|improve this question









New contributor




Rey Juna is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.




















  • Are the parameters more likely to be positive or negative? It's called many times in what context?
    – Mathieu Guindon
    2 hours ago








  • 1




    @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.
    – Rey Juna
    2 hours ago








  • 2




    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.
    – Mathieu Guindon
    2 hours ago










  • There really should never be a negative value. Maybe I am being over cautious, but I always try to cover the bases.
    – Rey Juna
    2 hours ago






  • 2




    I've rolled back the last edit, since it incorporates feedback from an answer. Please don't invalidate answers with edits! =)
    – Mathieu Guindon
    2 hours ago













up vote
2
down vote

favorite
1









up vote
2
down vote

favorite
1






1





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?










share|improve this question









New contributor




Rey Juna is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.











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?







vba excel






share|improve this question









New contributor




Rey Juna is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.











share|improve this question









New contributor




Rey Juna is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.









share|improve this question




share|improve this question








edited 2 hours ago









Mathieu Guindon

60.4k12147414




60.4k12147414






New contributor




Rey Juna is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.









asked 3 hours ago









Rey Juna

1114




1114




New contributor




Rey Juna is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.





New contributor





Rey Juna is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.






Rey Juna is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.












  • Are the parameters more likely to be positive or negative? It's called many times in what context?
    – Mathieu Guindon
    2 hours ago








  • 1




    @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.
    – Rey Juna
    2 hours ago








  • 2




    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.
    – Mathieu Guindon
    2 hours ago










  • There really should never be a negative value. Maybe I am being over cautious, but I always try to cover the bases.
    – Rey Juna
    2 hours ago






  • 2




    I've rolled back the last edit, since it incorporates feedback from an answer. Please don't invalidate answers with edits! =)
    – Mathieu Guindon
    2 hours ago


















  • Are the parameters more likely to be positive or negative? It's called many times in what context?
    – Mathieu Guindon
    2 hours ago








  • 1




    @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.
    – Rey Juna
    2 hours ago








  • 2




    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.
    – Mathieu Guindon
    2 hours ago










  • There really should never be a negative value. Maybe I am being over cautious, but I always try to cover the bases.
    – Rey Juna
    2 hours ago






  • 2




    I've rolled back the last edit, since it incorporates feedback from an answer. Please don't invalidate answers with edits! =)
    – Mathieu Guindon
    2 hours ago
















Are the parameters more likely to be positive or negative? It's called many times in what context?
– Mathieu Guindon
2 hours ago






Are the parameters more likely to be positive or negative? It's called many times in what context?
– Mathieu Guindon
2 hours ago






1




1




@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.
– Rey Juna
2 hours ago






@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.
– Rey Juna
2 hours ago






2




2




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.
– Mathieu Guindon
2 hours ago




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.
– Mathieu Guindon
2 hours ago












There really should never be a negative value. Maybe I am being over cautious, but I always try to cover the bases.
– Rey Juna
2 hours ago




There really should never be a negative value. Maybe I am being over cautious, but I always try to cover the bases.
– Rey Juna
2 hours ago




2




2




I've rolled back the last edit, since it incorporates feedback from an answer. Please don't invalidate answers with edits! =)
– Mathieu Guindon
2 hours ago




I've rolled back the last edit, since it incorporates feedback from an answer. Please don't invalidate answers with edits! =)
– Mathieu Guindon
2 hours ago










3 Answers
3






active

oldest

votes

















up vote
2
down vote













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





share|improve this answer























  • Oh well, there goes my draft! :)
    – Mathieu Guindon
    2 hours ago












  • @MathieuGuindon - Reading fail. Edited (although I'll stand by the rest of that bullet).
    – Comintern
    2 hours ago






  • 1




    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.
    – Mathieu Guindon
    2 hours ago






  • 1




    @MathieuGuindon That would make a good answer. ;-)
    – Comintern
    2 hours ago










  • Yeah, I like the Select Case approach - I was going to add it to my answer.
    – AJD
    2 hours ago


















up vote
1
down vote













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





share|improve this answer























  • 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.
    – AJD
    2 hours ago










  • @AJD, I like that approach. You should post as an answer.
    – Rey Juna
    2 hours ago










  • @AJD that's IIf ;-)
    – Mathieu Guindon
    2 hours ago






  • 1




    @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.
    – Comintern
    2 hours ago










  • @MathieuGuindon: thanks - yes IIf(). If() works in .Net as well and is safer.
    – AJD
    2 hours ago


















up vote
-1
down vote













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.






share|improve this answer























  • I did it again! If() instead of IIf()
    – AJD
    2 hours ago








  • 2




    SelValue = CVErr(xlErrValue) is only useful if the function is a UDF. I would absolutely Err.Raise 5 here and still return a Long.
    – Mathieu Guindon
    2 hours ago










  • 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.
    – Vogel612
    1 hour ago











Your Answer





StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");

StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");

StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});

function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});


}
});






Rey Juna is a new contributor. Be nice, and check out our Code of Conduct.










draft saved

draft discarded


















StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f209101%2ffunction-for-minimum-non-zero-with-multiple-additional-conditions%23new-answer', 'question_page');
}
);

Post as a guest















Required, but never shown

























3 Answers
3






active

oldest

votes








3 Answers
3






active

oldest

votes









active

oldest

votes






active

oldest

votes








up vote
2
down vote













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





share|improve this answer























  • Oh well, there goes my draft! :)
    – Mathieu Guindon
    2 hours ago












  • @MathieuGuindon - Reading fail. Edited (although I'll stand by the rest of that bullet).
    – Comintern
    2 hours ago






  • 1




    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.
    – Mathieu Guindon
    2 hours ago






  • 1




    @MathieuGuindon That would make a good answer. ;-)
    – Comintern
    2 hours ago










  • Yeah, I like the Select Case approach - I was going to add it to my answer.
    – AJD
    2 hours ago















up vote
2
down vote













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





share|improve this answer























  • Oh well, there goes my draft! :)
    – Mathieu Guindon
    2 hours ago












  • @MathieuGuindon - Reading fail. Edited (although I'll stand by the rest of that bullet).
    – Comintern
    2 hours ago






  • 1




    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.
    – Mathieu Guindon
    2 hours ago






  • 1




    @MathieuGuindon That would make a good answer. ;-)
    – Comintern
    2 hours ago










  • Yeah, I like the Select Case approach - I was going to add it to my answer.
    – AJD
    2 hours ago













up vote
2
down vote










up vote
2
down vote









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





share|improve this answer














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






share|improve this answer














share|improve this answer



share|improve this answer








edited 2 hours ago

























answered 2 hours ago









Comintern

3,68411124




3,68411124












  • Oh well, there goes my draft! :)
    – Mathieu Guindon
    2 hours ago












  • @MathieuGuindon - Reading fail. Edited (although I'll stand by the rest of that bullet).
    – Comintern
    2 hours ago






  • 1




    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.
    – Mathieu Guindon
    2 hours ago






  • 1




    @MathieuGuindon That would make a good answer. ;-)
    – Comintern
    2 hours ago










  • Yeah, I like the Select Case approach - I was going to add it to my answer.
    – AJD
    2 hours ago


















  • Oh well, there goes my draft! :)
    – Mathieu Guindon
    2 hours ago












  • @MathieuGuindon - Reading fail. Edited (although I'll stand by the rest of that bullet).
    – Comintern
    2 hours ago






  • 1




    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.
    – Mathieu Guindon
    2 hours ago






  • 1




    @MathieuGuindon That would make a good answer. ;-)
    – Comintern
    2 hours ago










  • Yeah, I like the Select Case approach - I was going to add it to my answer.
    – AJD
    2 hours ago
















Oh well, there goes my draft! :)
– Mathieu Guindon
2 hours ago






Oh well, there goes my draft! :)
– Mathieu Guindon
2 hours ago














@MathieuGuindon - Reading fail. Edited (although I'll stand by the rest of that bullet).
– Comintern
2 hours ago




@MathieuGuindon - Reading fail. Edited (although I'll stand by the rest of that bullet).
– Comintern
2 hours ago




1




1




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.
– Mathieu Guindon
2 hours ago




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.
– Mathieu Guindon
2 hours ago




1




1




@MathieuGuindon That would make a good answer. ;-)
– Comintern
2 hours ago




@MathieuGuindon That would make a good answer. ;-)
– Comintern
2 hours ago












Yeah, I like the Select Case approach - I was going to add it to my answer.
– AJD
2 hours ago




Yeah, I like the Select Case approach - I was going to add it to my answer.
– AJD
2 hours ago












up vote
1
down vote













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





share|improve this answer























  • 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.
    – AJD
    2 hours ago










  • @AJD, I like that approach. You should post as an answer.
    – Rey Juna
    2 hours ago










  • @AJD that's IIf ;-)
    – Mathieu Guindon
    2 hours ago






  • 1




    @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.
    – Comintern
    2 hours ago










  • @MathieuGuindon: thanks - yes IIf(). If() works in .Net as well and is safer.
    – AJD
    2 hours ago















up vote
1
down vote













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





share|improve this answer























  • 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.
    – AJD
    2 hours ago










  • @AJD, I like that approach. You should post as an answer.
    – Rey Juna
    2 hours ago










  • @AJD that's IIf ;-)
    – Mathieu Guindon
    2 hours ago






  • 1




    @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.
    – Comintern
    2 hours ago










  • @MathieuGuindon: thanks - yes IIf(). If() works in .Net as well and is safer.
    – AJD
    2 hours ago













up vote
1
down vote










up vote
1
down vote









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





share|improve this answer














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






share|improve this answer














share|improve this answer



share|improve this answer








edited 2 hours ago

























answered 2 hours ago









FreeMan

634515




634515












  • 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.
    – AJD
    2 hours ago










  • @AJD, I like that approach. You should post as an answer.
    – Rey Juna
    2 hours ago










  • @AJD that's IIf ;-)
    – Mathieu Guindon
    2 hours ago






  • 1




    @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.
    – Comintern
    2 hours ago










  • @MathieuGuindon: thanks - yes IIf(). If() works in .Net as well and is safer.
    – AJD
    2 hours ago


















  • 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.
    – AJD
    2 hours ago










  • @AJD, I like that approach. You should post as an answer.
    – Rey Juna
    2 hours ago










  • @AJD that's IIf ;-)
    – Mathieu Guindon
    2 hours ago






  • 1




    @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.
    – Comintern
    2 hours ago










  • @MathieuGuindon: thanks - yes IIf(). If() works in .Net as well and is safer.
    – AJD
    2 hours ago
















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.
– AJD
2 hours ago




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.
– AJD
2 hours ago












@AJD, I like that approach. You should post as an answer.
– Rey Juna
2 hours ago




@AJD, I like that approach. You should post as an answer.
– Rey Juna
2 hours ago












@AJD that's IIf ;-)
– Mathieu Guindon
2 hours ago




@AJD that's IIf ;-)
– Mathieu Guindon
2 hours ago




1




1




@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.
– Comintern
2 hours ago




@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.
– Comintern
2 hours ago












@MathieuGuindon: thanks - yes IIf(). If() works in .Net as well and is safer.
– AJD
2 hours ago




@MathieuGuindon: thanks - yes IIf(). If() works in .Net as well and is safer.
– AJD
2 hours ago










up vote
-1
down vote













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.






share|improve this answer























  • I did it again! If() instead of IIf()
    – AJD
    2 hours ago








  • 2




    SelValue = CVErr(xlErrValue) is only useful if the function is a UDF. I would absolutely Err.Raise 5 here and still return a Long.
    – Mathieu Guindon
    2 hours ago










  • 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.
    – Vogel612
    1 hour ago















up vote
-1
down vote













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.






share|improve this answer























  • I did it again! If() instead of IIf()
    – AJD
    2 hours ago








  • 2




    SelValue = CVErr(xlErrValue) is only useful if the function is a UDF. I would absolutely Err.Raise 5 here and still return a Long.
    – Mathieu Guindon
    2 hours ago










  • 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.
    – Vogel612
    1 hour ago













up vote
-1
down vote










up vote
-1
down vote









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.






share|improve this answer














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.







share|improve this answer














share|improve this answer



share|improve this answer








edited 2 hours ago

























answered 2 hours ago









AJD

1,1881213




1,1881213












  • I did it again! If() instead of IIf()
    – AJD
    2 hours ago








  • 2




    SelValue = CVErr(xlErrValue) is only useful if the function is a UDF. I would absolutely Err.Raise 5 here and still return a Long.
    – Mathieu Guindon
    2 hours ago










  • 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.
    – Vogel612
    1 hour ago


















  • I did it again! If() instead of IIf()
    – AJD
    2 hours ago








  • 2




    SelValue = CVErr(xlErrValue) is only useful if the function is a UDF. I would absolutely Err.Raise 5 here and still return a Long.
    – Mathieu Guindon
    2 hours ago










  • 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.
    – Vogel612
    1 hour ago
















I did it again! If() instead of IIf()
– AJD
2 hours ago






I did it again! If() instead of IIf()
– AJD
2 hours ago






2




2




SelValue = CVErr(xlErrValue) is only useful if the function is a UDF. I would absolutely Err.Raise 5 here and still return a Long.
– Mathieu Guindon
2 hours ago




SelValue = CVErr(xlErrValue) is only useful if the function is a UDF. I would absolutely Err.Raise 5 here and still return a Long.
– Mathieu Guindon
2 hours ago












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.
– Vogel612
1 hour ago




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.
– Vogel612
1 hour ago










Rey Juna is a new contributor. Be nice, and check out our Code of Conduct.










draft saved

draft discarded


















Rey Juna is a new contributor. Be nice, and check out our Code of Conduct.













Rey Juna is a new contributor. Be nice, and check out our Code of Conduct.












Rey Juna is a new contributor. Be nice, and check out our Code of Conduct.
















Thanks for contributing an answer to Code Review Stack Exchange!


  • Please be sure to answer the question. Provide details and share your research!

But avoid



  • Asking for help, clarification, or responding to other answers.

  • Making statements based on opinion; back them up with references or personal experience.


Use MathJax to format equations. MathJax reference.


To learn more, see our tips on writing great answers.





Some of your past answers have not been well-received, and you're in danger of being blocked from answering.


Please pay close attention to the following guidance:


  • Please be sure to answer the question. Provide details and share your research!

But avoid



  • Asking for help, clarification, or responding to other answers.

  • Making statements based on opinion; back them up with references or personal experience.


To learn more, see our tips on writing great answers.




draft saved


draft discarded














StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f209101%2ffunction-for-minimum-non-zero-with-multiple-additional-conditions%23new-answer', 'question_page');
}
);

Post as a guest















Required, but never shown





















































Required, but never shown














Required, but never shown












Required, but never shown







Required, but never shown

































Required, but never shown














Required, but never shown












Required, but never shown







Required, but never shown







Popular posts from this blog

Accessing regular linux commands in Huawei's Dopra Linux

Can't connect RFCOMM socket: Host is down

Kernel panic - not syncing: Fatal Exception in Interrupt