4

In an oft-cited (if dated) reference for C# coding standards (pdf; from Lance Hunt), the statement is made,

33. Avoid invoking methods within a conditional expression.

I've not seen this recommendation elsewhere, including in the .NET guidelines (here or here). Unlike the other very reasonable guidelines in Hunt's document, this suggestion seems unintuitive to me. Can someone provide a justification for it?

9

Well, I did not read Hunt's document in full but it seems that his advice is mainly about readability. A statement like...

if(myObject1.functionA() && myObject1.functionB())
{
  // do something
}

...has a certain risk of being misunderstood, because of short-circuit evaluation, and also because of the if-line length. Instead...

bool resultOfA = myObject1.functionA();
bool resultOfB = myObject1.functionB();

if(resultOfA && resultOfB)
{
  // do something
}

...is easier to read, and will always call both functions(). Of course, if you explicitly want to make use of short-circuit evaluation (maybe because functionB shouldn't be called when functionA returns false), then you may be better off ignoring Hunt's rules (note: he says "avoid", not "don't do this under any circumstances").

  • 1
    Ahh, I misunderstood Lance's statement altogether. (I thought he was discouraging if (x) doThis() else doThat(), which of course would be a completely untenable restriction.) Perhaps he could have reworded his guideline as "Avoid invoking methods within a boolean expression", but then again, I'm probably the only doofus who misunderstood. Thanks for a complete and thoughtful answer. – kmote Jun 16 '14 at 16:34
  • @kmote I think its more related to short circuit evaluation done in Java, C# etc. - using functions this way would be less confusing if the language always fully evaluates all sub-expressions. – Alok Jun 16 '14 at 17:11
  • 3
    @Alok: actually, I am in doubt about that. Short-circuit evalution - when applied correctly - can improve the readability of certain code. I use it very often in statements like if (foo != null && foo.bar == x) // do something to avoid deep indentation. – Doc Brown Jun 16 '14 at 18:01
  • If foo.bar == x was a method call, it would never be invoked - hence the warning mentioned in the question imho. Its possible Hunt is only referring to readability but that seems a lesser issue than correctness in this case. – Alok Jun 16 '14 at 18:11
  • @Alok: I think you are smart enough to anticipate an example with a function call involved and improved readability because of short-circuit evaluation. – Doc Brown Jun 17 '14 at 6:15
5

You should also consider function calls that have side effects (e.g. change some global variables). This can be a more important issue than just readability.

if (funcA() && funcB()) ...

It might be intended that funcB only gets called when funcA succeeds, but you also might do a mistake here. If both functions have side effects and both are needed to be executed, then the code above is logically flawed. This may happen because the programmer was too ambitious with keeping his code tight, but it could also be that it was correct before, but due to some changes somewhere else the assumptions taken are no longer valid.

Therefore it's never a good idea to call functions with side effects inside a boolean expression.

  • 1
    Global variables are evil for this reason. – JBRWilkinson Jun 16 '14 at 22:13
  • 2
    it's not just that though. no matter what, if funcB must be executed for some reason, then the code above is flawed – Philipp Murry Jun 16 '14 at 22:22
  • @PhilippMurry: and if funcB must not be executed, without short-circuit evaluation the code would be flawed, too - you can always turn the argument around, this is very opinionated. Actually, the more I think about Hunt's rule, the less I like it. My rule would be: use functions in conditionals with care, only when it is safe and when it improves readability. – Doc Brown Jun 17 '14 at 6:19
  • "without short-circuit evaluation the code would be flawed, too" - no, because you probably would take care of that. i assume that if you know that one call depends on the other, you would do a proper check. it's just more likely to oversee things inside a conditional expression. in one of my CS classes you weren't allowed to have program logic inside a boolean expression. – Philipp Murry Jun 17 '14 at 21:56
  • @PhilippMurry: I think david.pfx's example shows perfectly how functions in conditionals can be used with care (and how not). And CS classes are not famous for always teaching the most pragmatic solutions. – Doc Brown Jun 18 '14 at 12:59
4

First, it is clear from the context that by conditional expression Hunt means the boolean expression in a conditional statement. This is a section about flow control, and not about the use of boolean expressions for other purposes, or about (what I would call) conditional expressions based on the ternary '?:' operator.

I believe the underlying intent here is that the parts of a conditional expression used for flow control should avoid misunderstandings that might arise from shortcircuit evaluation or from side-effects. I can see no reason for avoiding calling method functions to obtain values used in a conditional expression (provided there are no side-effects), and I can see no problem in depending on shortcircuit evaluation provided the relationships between the parts of the expression are clearly visible. It's the risk of hidden connections and hidden consequences that should be avoided. So:

if (x.Contains(a) && x.Median() > b) {} // ok
if (x.Insert(a) && x.Median() > b) {} // NOT ok

There is a further consideration. The calling of a method may involve a performance penalty, so if Median() is slow to compute:

if (x.Median() >= a && x.Median() <= b) {} // slower
var median = x.Median();
if (median >= a && median <= b) {} // faster

Having said all that, I think Hunt does us a disservice by failing to explain what he means. Hopefully a future release will address that shortcoming.

  • I appreciate your point of view and agree with your conclusions. I also agree that Hunt's excellent guide (which I use and recommend to others) is due for an updated version. Are you listening Lance? – kmote Jun 17 '14 at 15:30
3

It encourages loading multiple function-calls into the conditional, and then relying on short-circuit evaluation to handle branching.

When this happens your program becomes harder to debug. If you have 5 functions within a single conditional and all you know is that the conditional failed... Then your forced to step-debug the conditional, or spam break-points to see what's actually being called. In fact, it's probably faster to refactor at that point and place each function call on its own line.

It seems harmless at first, and yes you will be saving lines. But when your working on a large project loaded with this style, or your project grows and you've been using this style, it's going to add time to your debugging efforts. You should program like it's going to break, because it's going to break.

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.