3

When I want to execute some code under the condition that a variable has one of two (or more) values I can use the OR operator:

if (var == x || var == y) {
    DoSomething();
}

But I'm not sure whether I should do this directly, when var is a getXY() function, that might even include some expensive checks:

// Is this good style?
if (getXY() == x || getXY() == y) {
    DoSomething();
}

Is there some specified behaviour whether or not the compiler is optimizing these two function calls to only one for both checks? Or is this implementation specific and I have to check for each case?

Would it be "better" to do the function call first, store the result in var and then do the check on var? What is in general considered good coding style for this?

// Or should I do the check first?
var = getXY();
if (var == x || var == y) {
    DoSomething();
}

I'm programming in C, but I would assume this to be a general consideration similar in most languages.

Improve this question
4
  • 4
    In general, the compiler won't be allowed to optimize out the duplicate function call unless it can prove it wouldn't have had any side-effects. If it logs something, or changes any global state, both calls must happen as written. Commented Dec 21, 2020 at 14:28
  • 1
    "Is there some specified behaviour whether or not the compiler is optimizing these two function calls to only one for both checks?" - only if the function is pure, which the compiler probably will only find out if it is inlined. In general, just use the second approach with the temporary variable. Commented Dec 21, 2020 at 20:03
  • And remember that even a function like sin(x) is not pure - in the right situation it sets errno. (Actually, the function doesn't need to be pure to remove duplicate function calls; as long as it is idempotent all but the first call can be removed). Commented Dec 23, 2020 at 0:36
  • The only possible hesitation for declaring a temporary variable is if the code base or C compiler is C89 or pre-C89. In those cases, I would worry more about (1) outdated code base containing unfixed vulnerabilities, or worse, (2) outdated compilers with known vulnerabilities. In general, if permitted, temporary variables should be used whenever it is helpful, and declare those as close as possible to where it is used. Commented Dec 23, 2020 at 2:34

4 Answers 4

Reset to default
9

If you write down one call to getXY() in your code, and I read your code, then I know what happens: There is one call to getXY(), and if the result is either x or y then DoSomething() gets called.

If you write down two calls to getXY() in your code, and I read your code, then things are more complicated: There is one call to getXY() first. If the result is x, then DoSomething() gets called. Otherwise, there is another call to getXY(), and if the result is y, then DoSomething() gets called.

Now without reading the source code for getXY(), I don't know if calling getXY() twice has any side effects. For example, getXY() might increment a static variable and return it. In that case, I can't swap the order of the comparisons with x and y. Or x or y might be a static variable, and calling getXY() increases it. It has nothing to do with efficiency, but with code complexity. If one call is enough, use one call.

Improve this answer
4

I think it comes down to this: do you know (does it seem) like this specific action should be extremely costly?

If so, then maybe it is a good idea to move it out of your condition, maybe even wrap it into something asynchronous or exception-catching (depending on specific language and libraries used).

I think your problem actually implies a better question: should expensive code be located in get-styled methods/functions in the first place?

Coming from object-oriented languages (not so sure about C conventions, specifically), I would consider this to be a bad practice that should be avoided.

Get implies minimal calculations (even none, if we are talking about something like C#'s auto-implemented getters in properties) and quick access: like retrieving a value from a field/variable.

If the method is more complex than that, then this should be reflected in its name. Code inside isn't just "getting" the value from the storage room anymore. It is now sending a courier on horseback into a neighboring town. That's not "getting". That's "delivering", or "acquiring", or "generating", or something else that implies lengthy process.

As for compiler optimization, my guess is—it depends on a specific compiler.

Improve this answer
1
  • Fair enough, I guess you have a good point there. Getters should be simple. But this is not exactly answering the question in general. Even with simple getters, the question still holds. Calling the function two times? Or use a temp variable? Commented Dec 22, 2020 at 9:13
2

IMHO the focus should not be on "style" here, but on not violating the DRY ("Don't Repeat Yourself") principle.

This code

if (getXY() == x || getXY() == y) {
    DoSomething();
}

is not as DRY as it could be. For new equirements where getXY() needs to be replaced by something different, there will be two places to change instead of one (even if those places are currently on the same line). However,

var = getXY();
if (var == x || var == y) {
    DoSomething();
}

makes sure there is only one place left to change, for almost no additional costs. That is the reason this would be my first choice of implementation. The fact this second variant might be more efficient is just a minor "bonus" of this approach (but since it is unlikely to become less efficient, the second variant does not introduce any performance risks).

Note also in case getXY() is not side-effect free, or can deliver different values between two consecutive different calls, both variants might be semantically different and one has to decide which one is the "correct" one (but I guess this is not the scenario you had in mind with this question).

Improve this answer
5
  • @Heagon. DRY is about maintainability and evolvability, about making software less error prone and easier to extend. By the term "style" I understand things like formatting of the source code, naming and case conventions, rules of indentation or usage of brackets. This can also improve maintainability, but has IMHO a slightly different focus. Of course, the term "style" is not strictly defined, and your understanding may be a different one. Commented Dec 21, 2020 at 16:21
  • If the getXY() call is not taking any arguments, but is really just an identifier followed by parenthesis, there's not much to repeat compared against writing var twice. I would prefer the more concise solution here (if I knew about purity of the function). I'll invoke YAGNI on the temporary variable, the original code is already pretty dry. Commented Dec 21, 2020 at 20:08
  • @Bergi: fair enough. Commented Dec 21, 2020 at 20:11
  • Firstly, there are two parties involved here: the caller needs to know that getXY is free of side effects and the maintainer of getXY needs to know that the caller relies on it, too. Secondly, coming from a background in safety critical systems, I always wondered why no one ever questioned the use of if-statements in general. I’d prefer a language that abandons if and replaces it with switch statement (with an appropriate syntax to ease its use), but I don’t know of any. (The case-statement of the Ada language comes close to it, but is still too cumbersome to replace if.) Commented Dec 22, 2020 at 8:33
  • I kind of agree with Doc Brown here, I like the DRY principle. For two values to check it might not make a big difference, but I have seen some conditional statements, where the getter was compared to about 6 or 7 different values. In this case it gets really confusing, especially with a long name for the getter. Commented Dec 22, 2020 at 9:19
0

When wondering about coding style, it sometimes help to think in terms of real-world algorithmic operations.

A thirsty person walks by a convenience store, with only one coin in their pocket. Water bottles cost 50-cents (for all intents and purposes of this answer). Which one of the following would be the most typical reaction?

Option 1: Take the coin out of pocket and check if it is a 1$ coin or a 50-cent coin... if so, get in store and buy a bottle of water.

Option 2: Take the coin out of pocket, check if it is a 1$ coin, if it is, go buy a bottle of water. If not, then put it back in the pocket. Then take it out again, check if it is a 50-cent coin. If so, go buy a bottle of water.

The difference between the two? Operationally, absolutely none. Our person wouldn't really suffer terminal dehydration in between picking a coin out and putting it back in their pocket a couple of times. But this doesn't mean Option 2 is a good choice. More words to formulate, more actions than absolutely necessary, more thought involved... However, if you can live with that added quantum of complexity, there really is no reason to be too religious about it. The difference should be minor (unless you have a really deep pocket).

While not so simplistic in real life, in programming, the option that raises the least eyebrows is usually the way to go.

Improve this answer
2
  • But what if, in Option 2, you pull out a different coin? You might get a 50c the first time, and a $1 the second (and hence have no drink!) Commented Dec 23, 2020 at 16:12
  • @Andrew Yes, this was also part of my point. You either have one coin, in which case Option 2 makes less sense (though you will, eventually, get a drink), or you have many coins, in which case Option 2 can also leave you without drink. OP suggests: if (getXY() == x || getXY() == y) ... what would happen if the first getXY() call returned y and the second getXY() call returned x? ..... No drink either! Commented Dec 24, 2020 at 2:04

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.