2

I have a value that cannot go below a set value or above another set value. I have written a method to handle this for me. I am showing you two version of this same method written slightly different. One with multiple if statements and one with an else if tree. Both return the same result. But is one better than the other for some reason? Or (no kidding) is there a third, better way?

double Constrain(double low, double value, double high)
{
    if (value < low)
        return low;
    else if (value > high)
        return high;
    else
        return value;
}

double Constrain(double low, double value, double high)
{
    if (value < low)
        return low;
    if (value > high)
        return high;
    return value;
}
Jerry Nixon
  • 31,313
  • 14
  • 117
  • 233
  • 5
    I don't see any reason why one would be better than the other. The execution path will be exactly the same. I would prefer the second method because it's more terse, but that's more of a personal opinion – Kenneth Jan 14 '16 at 19:45
  • I know Resharper would recommend changing the first one to the second one. And then changing the last two lines of the second to `return value > high ? high : value;` – juharr Jan 14 '16 at 19:45
  • You could do... return ((valuehigh?high:value));... but I am being facetious. Your two methods are totally equivalent, neither is better than the other. Even readability is unaffected. –  Jan 14 '16 at 19:46
  • 1
    If it was possible, I'd definitely vote against the close votes: The question is **not** primarily opinion-based, because Jerry explicitly asked for possible alternatives, and he didn't define what he means by "better", so this leaves room for interpretation. If you not only constrain ;-) yourself to the two choices given, but think about other ways of handling this, I don't see this as primarily opinion-based. – Golo Roden Jan 14 '16 at 19:48
  • 3
    Might want to add a check that `low` is less than or equal to `high`. – juharr Jan 14 '16 at 19:51
  • The methods are very clean... There's no reason we should address readability. My question is "Why optimize this at all?" Unless this is being called really, really often, there are more important areas to optimize. As far as the performance equivalence, I'd bet some money that if you compiled both of those functions in release mode and decompiled them, they'd be the exact same IL. Probably even in debug mode. And if you're just bored: http://stackoverflow.com/questions/17095324/fastest-way-to-determine-if-an-integer-is-between-two-integers-inclusive-with – Millie Smith Jan 15 '16 at 07:19

5 Answers5

3

Is one better? Performance is seemingly not an issue, so the question becomes which is more readable and maintainable? If you use ReSharper, it will flag the first one as having unnecessary elses. If you don't use ReSharper, it should come down to which one YOU and YOUR TEAM find the easiest to understand.

I was originally going to suggest using max(min(input,high),low). I decided not to because it is more difficult to understand (imo).

Kyle W
  • 3,702
  • 20
  • 32
1

Not really the answer to your question but since we have a good conversation here in answers I would add this version as a more generic code snippet:

public static T Constrain <T>(this T val, T low, T high) where T : IComparable<T>
{
    if (val.CompareTo(low) < 0) return low;
    if (val.CompareTo(high) > 0) return high;
    return val;
}
serhiyb
  • 4,753
  • 2
  • 15
  • 24
0

These are virtually the same. If the value is in between the low and the high values then it will have to do two checks and the return statement on both ways. If the value is higher than the high value then both will have to do two checks (the low check is first) and then the return statement. If the value is lower than the low value then both will have to do one check and a return statement.

Sam Williams
  • 162
  • 13
0

Ok, maybe not facetious. Here is how I would do it

function constrain (low, value, high) {
    return (value<low? low:
            value>high? high:
            value);
}
  • 1
    @MoH. That depends on how your team feels about ternary operators. I find this very easy to parse, but if you want to emphasize usability for new developers who may not understand it, then it might not be as great of an option. – Kyle W Jan 14 '16 at 19:54
  • @KyleW I agree, its a very 'whatever works for you' kind of statement. I generally prefer my code be as readable as possible. Yes my team may like it and be able to understand it, but what happens if they move to different teams and are replaced. I prefer that anyone be able to understand my code with as little difficulty as possible because reading other peoples code tends to be tedious, lets not make it any harder. I'm not saying Agapwlesu's answer is bad by any means, just a little bit less readable. – Mo H. Jan 14 '16 at 19:59
  • 1
    @MoH. - agree with KyleW. But I would add that all programmers should be extremely familiar with the ternary operator - familiar enough to look at that and read it without pause. Gives you a single exit point; it's quick and to the point. I find the ternary operator to be very useful... within reason. If I hired a new programmer that did not get that, I would expect him/her to learn to get it, while staying away from expressions with 20 ternary operators in one command. –  Jan 14 '16 at 20:03
-1

I have a value that cannot go below a set value or above another set value.

So this means that it is up to the programmer to call Constrain whenever they are working with this value - which sounds like a simple mistake that could be easy to make.

If you have the scope for it, you could approach this is a more object orientated way and create a class that clamps the double value. That way you can work with it as you please, and it will never exceed the limits.

TVOHM
  • 2,740
  • 1
  • 19
  • 29