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.

  • bool loginSuccessful = (NetworkAvailable()) ? LogUserOn() : false;

    Clearly indicates intention, and saves those precious code lines for future generations.

  • @commentors:  For what it's worth, I suspect this blog post was inspired by the following link:  http://stackoverflow.com/questions/1544861/which-code-is-more-readable

    In the original context of the question, I ultimately opted for something like:

    OptionEnabled = IsAllowedToDoX() && CanDoX();

    b/c neither method has side effects and CanDoX() was potentially much more expensive.

  • { diagnostic trace 01 }

    bool canUseCloud = false;                

    canUseCloud = NetworkAvailable();

    if (canUseCloud)  

     {

           { diagnostic trace 02 }

     canUseCloud = UserHasFreeSpaceInCloud();   //should actually return the amount of free space so y ou can use it for diagnostic purposes --- a ZERO means no free space -- a -1 is an error with err number returned somewhere else

     }

    { diagnostic trace 03 }

    ==> canUseCloud always has a value even during exceptions in either of the methods called

    ==> trace messages show exactly what code path was taken

    ==> can be slightly enhanced for timing trace purposes such as 'how long did it take to see if we could get network access?'  and 'How long did it take to check for free resources in the cloud?'

    Network usage is a critical trace point in applications and poor diagnostics or lack thereof results in a large increase in support cost for a production application.

  • Once I realized you were talking about Command Query Separation (CQS) it all made sense.

    LogUserOn is a Command and should have no Query semantics.

  • I am amused (in a sort of "huh, that many people don't usually agree with me" way), and gratified, that most of the comments are proposing exactly the third alternative I would have.

    I see the reasoning behind wanting to put the LogUserOn() method call in a statement on its own, even if I'm reasonably comfortable with the && expression myself.  But there was still something about the proposed version that made me uneasy, until I mentally refactored the "else" out of the code.

    Sure, pre-initializing the boolean might be less efficient.  But a), since locals are initialized anyway, maybe the JIT compiler will eventually make that a NOP (if it doesn't already), and b) what's a quick assignment between friends if the code winds up looking nicer, and is easier to maintain, as a result?

    Bonus: my secret code for posting the comment is only the greatest airliner ever built, the 747!  :)

    Bummer: turns out, that's the code that was valid half a day ago, when I first stuck this article in my reading queue.  Now, the code is only the area code for the state where I grew up.  Oh well.  :(

  • I imagine your preferred semantics would have "won" had the original designers specified && to be a short-circuit operator -without- guaranteed order.  For instance, by saying that if the right-hand expression can be compile-time reduced to false, it's OK to skip the test.  Or that compiler hints could cause the fastest test to be performed first, regardless of whether it's left-hand or right-hand side.  Etc.  Then you'd only use && if the operation was truly commutable, with no side effects.

    But as it is, the operator has a known, fixed meaning.  The semantics -are- the syntax...

  • I would use the first examples in both cases (if - else).  To me, code readability and simplicity of debugging is paramount in a company like mine with medium to large development teams, where anyone could pick up anyone else's code in the future.  It's easier to step-by-step describe your intentions with the if-else examples.  

    If I were assigned to debug or maintain someone else's code, I would rather to see the if-then.  Personal preference.

    I would write it this way - clear intentions, no if-then, no &&.

    bool CanUseCloud()

    {

     if (!NetworkAvailable())

       return false;

     return UserHasFreeSpaceInCloud();

    }

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

    For me && should be used only for value decision, and not for something that engages operations.

    If && engages a functional operations, then it should be separated, or integrated to be value based.. or better, throw exceptions:

    bool canUseCloud = CloudIsAccessible();

    CloudIsAccessible() { if( NetworkAvailable() )... }

  • It took me a while to pick it up, but I think the scent trail of this code smell actually leads, ultimately, to the method LogUserOn(), and the fact it returns a bool to indicate success or failure.

    If it returned another type, you couldn't even consider applying the shortcircuit pattern. If it returned a LogonResult object, would you still feel as uncomfortable if the code read

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

    ?

    There, the side-effect causing method is even more deeply buried, yet I'm not sure I feel as uncomfortable seeing it.

    It's interesting, to me, that I would be totally comfortable seeing the shortcircuiting version of the code in JavaScript. I think the reason is that JS's concept of 'truthiness' permits the pattern to be applied to a wider variety of functions with a wider variety of return types and values, rather than it simply being a trick you can play if a method happens to return a Boolean to indicate success.

  • I for one would prefer:

    bool loginSuccessful = false;

    if (NetworkAvailable())

    {

       loginSuccessful = LogUserOn();

    }

    with the same reasoning as other have pointed out (i.e. readability and tool breakpoints)

    But additionally, I like it better because as the code matures, there may be other actions you need to do when the network is available but with no influence on whether the user logged on successfully.  So, if the code changed to:

    bool loginSuccessful = false;

    if (NetworkAvailable())

    {

       loginSuccessful = LogUserOn();

       SendUsageInfo();  // or something network related

    }

    ... then the delta between versions would be relatively minimal (versus breaking out the && into a more nested statement like this)... which would make it easier to do various things like determining how the code has evolved (history), merging, maintenance, etc.

  • My favorite option for the login example is definitely:

    bool loginSuccessful = false;

    if (NetworkAvailable())

     loginSuccessful = LogUserOn();

    What I like about this is that it can very safely be refactored into:

    bool loginSuccessful = false;

    try

    {

       if (NetworkAvailable())

         loginSuccessful= LogUserOn();

    }

    catch(Exception ex)

    {

      // handling

    }

    The initialization statement clearly defaults a user to not being logged in, and the succeeding logic makes it clear that the will only be logged in if the network is available and the user is successfully logged on. I don't find short-circuiting to be evil, but I think should be reserved for more straight-forward cases like the example Pavel mentioned above.  Since (I assume) the LogUserOn() method is complex, I like it being a statement for me to easily step into by placing a breakpoint at the call site.  If it were written as:

    loginSuccessful = NetworkAvailable() && LogUserOn();

    the breakpoint would be required to be placed inside the LogUserOn method, or I would have to step into the NetworkAvailable() method first.  I like having the option.

    Also, I believe C# also supports boolean expressions such as:

    loginSuccessful = NetworkAvailable() & LogUserOn();

    The single ampersand will not short-circuit and the LogUserOn() method would be called without an available network.  This would likely cause an exception to be thrown in the LogUserOn() method.  The code would appear to gracefully handle an unavailable network, but would not.  This is a major deviation from the expected behavior from what is probably a very subtle syntactic difference.

  • This is a textbook case for moving code into a separate method; I would take Don B's approach without a second thought.  I also agree with RichB:  commands and queries really don't belong on the same line.  The arguments for separation far outweigh those against.  It boils down to personal preference, but there is great virtue in humble sensibility.

  • I use the former while coding but then in the reafractoring session all such is refractored to later.

    This is probably the old way of doing it, The way doding practice and approwch is evoloving (eg LINQ), I think mor and more people are shiftig towards the scond style.

    Another thing is, the braces arrownd the statements following conditions, I was not used to it initially with single statement following the condition, realised later that spending 1 second to enclose code in braces, in the first place could save me from  a lot of hastle and pain later.

  • Any decently monitored application would output

    - time to check network availablitiy

    - time to logon user to the cloud

    - amount of free resources on the cloud (if easy to compute)

    This leads to easy system monitoring for problem events

    - Network is not available for X minutes

    - User login takes longer than 30 seconds

    - Failed logons within the last 5 minutes > 100

    It also leads to easy monitoring outside of the application itself

     - test the login time duration once every 5 minutes

     - verify that a user can logon once every 5 minutes

    Tracking the above 5 monitoring test scenarios will be much easier than diagnosing problems after you get a telephone call from the end user.

  • > Once I realized you were talking about Command Query Separation (CQS) it all made sense.

    > LogUserOn is a Command and should have no Query semantics.

    Pure CQS as practiced by the hardcore Eiffel community is rather cumbersome, though. I don't see anything wrong with commands reporting their success or failure directly - separating that in  a query often necessitates an extra object just to carry the state, and is much more prone to race conditions.

Page 2 of 3 (44 items) 123