Style follows semantics

Style follows semantics

Rate This
  • Comments 44

Which is better style?

bool abc;
if (Foo())
  abc = Bar();
else
  abc = false;

vs

bool abc = Foo() && Bar();

?

To me, this comes down to the question “is Bar useful solely for obtaining its value, or also for its side effects?” The stylistic choices should typically be driven by a desire to clearly communicate the semantics of the program fragment.

The metasyntatic names are therefore making this harder to answer, not easier. Suppose the choice were in fact between:

bool loginSuccessful;
if (NetworkAvailable())
  loginSuccessful= LogUserOn();
else
  loginSuccessful= false;

and

bool loginSuccessful= NetworkAvailable() && LogUserOn();

I would always choose the former, because I want LogUserOn to be in a statement of its own. Statements emphasize “I am useful for my side effects”. Statements emphasize control flow and provide convenient places to put breakpoints.

If however the choice were between

bool canUseCloud;
if (NetworkAvailable())
  canUseCloud = UserHasFreeSpaceInCloud();
else
  canUseCloud = false;

and

bool canUseCloud = NetworkAvailable() && UserHasFreeSpaceInCloud();

I would always choose the latter; here we’re using && idiomatically to compute a value safely.

  • I think I agree with your analysis. One minor nit is that I would definitely use braces around each branch of the if/else version, even though they only contain single statements - IIRC StyleCop enforces such a style by default.

  • I'd always use the latter, as it's the simpler representation of the logic, but I'd add a comment describing the effect I'm after in the event that it's not self-evident to another dev.

  • I'd probably write

    bool loginSuccessful = false;

    if (NetworkAvailable())

     loginSuccessful = LogUserOn();

    so that I'm not tempted to use &&, which I agree is significantly less obvious when side effects matter.

  • I'm with Eric on both cases, but just throwing another option out there:

    bool loginSuccessful = false;

    if (NetworkAvailable()) {

     if (LogUserOn()) loginSuccessful = true;

    }

  • Hmmm, I don't believe I would combine two method calls into a boolean intialization statement, but I've changed my opinion on various coding mannerisms before, so I can't rule it out completely. But, for now, I would do

    bool myBool = criteria1 && criteria2;

    but not

    bool loginSuccessful = NetworkAvailable() && LogUserOn();

    For one, trying to log the user on should only occur if there is a network available for the attempt, and the syntax of

    bool loginSuccessful = false;

    if (NetworkAvailable())

       loginSuccessful = LogUserOn();

    Makes that more obvious. And, like Stuart, I initialize the boolean to false and then change it only with the result of LogUserOn(). Yes, it's three lines compared to single-line option, but it's more obvious to me the proper control flow. The same would go for the canUseCloud scenario.

  • I'd probably go with:

    bool loginSuccessful = false;

    if (NetworkAvailable())

    {

     loginSuccessful= LogUserOn();

    }

    I like to lift as much out of conditionals as possible.

  • I am not sure that I agree - I think the more compact approach in all three examples is the best expression of intention.  Given the fact that logical short circuiting makes both code examples fundamentally identical I think that the semantics of both read the same way (although I find the more compact approach more readable but to each his own).

    I don't think it is necessary to put a method call in its own statement to indicate that it is useful for its side effects.

  • Thinking about it again, I think my gut instinct would be to actually write it like so (although it's something that won't work in VB)...I tend to write these because the logic is a bit clearer, but it's just more compact (I tend to init my vars inline if I can):

    Boolean myBool = NetworkAvailable() ? LogUserOn() : false;

  • I find that my choice is more influenced by how LISP-y I'm feeling when I write my code. In both cases I'd probably be more likely to use the ternary operator. Expression are almost universally more useful than statements.

  • I think it depends on the level that the programmers are at; at the workplace. By all means the easier of the two if they are 1337 haxxors. If they have a pinch of experience maybe the shorter (split over two lines to aid SCM).

  • Although I D want to RY,

    with the first it easier to put a breakpoint on...

  • I disagree.  Code density doesn't necessarily lead to readability, and often hurts maintainability.

    Using "&&" where it might short-circuit evaluation of extra code is a very subtle pattern that can lead to very subtle bugs.

    I consider it a code smell when && and || are used outside of if statement clauses. And those clauses should have pretty obvious side effects. It looks like it's being used here more to be 'clever' than to make the code more readable. In Eric's case, I would be careful that UserHasFreeSpaceInCloud() could have side effects (especially since a method was chosen over a property).

    In general I dislike use of the tertiary operator as well, if statements have very clear code flow.

  • > Using "&&" where it might short-circuit evaluation of extra code is a very subtle pattern that can lead to very subtle bugs.

    I disagree with this in the broadest sense, because there is a particular pattern that is extremely common in languages such as Java in C#, which relies on short-circuiting; namely, null-checking. I.e.:

      if (x != null && x.Foo == bar) ...

    Short-circuit evaluation is crucial in this pattern, and it is widely used and understood, so I don't see anything wrong with it. It is similarly applied to any kind of range/validity checks, where right-hand side would otherwise throw if called.

    I think the actual problematic case is only when the right operand of && is an expression with intentional side effects (throwing NullReferenceException etc technically is one as well, but one could argue that it should never be intentional in well-written code, and hence doesn't count) - i.e.:

      if (DoSomething() && DoSomethingElse()) {

         // rely on side effects from DoSomethingElse()

         ...

      }

  • I used to think "shorter is always better", but using breakpoints in VS made me change my mind. It's a lot less easy to break after Foo() but before Bar() when using the && one-liner.

    So, both semantics and tool support can influence style.

  • Ease of breakpoints is about the only reason I would choose the first over the second. Stylistically, I think the && more clearly expresses the intent in both of the examples: loginSuccessful is dependent both on the network being available and the login being completed successfully. The other options presented by the post and in the comments do not make that clear. Of course any programmer who has experience in the language and is looking purposefully at the code could decipher the meaning of any of the fragments. But the whole point of stylistic choices is to account for things like programmer experience and attention span.

Page 1 of 3 (44 items) 123