invoking methods within a conditional expression

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?

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”).

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.

6

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.

1

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.

Trả lời

Email của bạn sẽ không được hiển thị công khai. Các trường bắt buộc được đánh dấu *