Style follows semantics


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.

Comments (44)

  1. Tony Cox [MSFT] says:

    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.

  2. Keith Patrick says:

    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.

  3. Joren says:

    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.

  4. I’m with Eric on both cases, but just throwing another option out there:

    bool loginSuccessful = false;

    if (NetworkAvailable()) {

     if (LogUserOn()) loginSuccessful = true;

    }

  5. Anthony P says:

    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.

  6. Mike Greger says:

    I’d probably go with:

    bool loginSuccessful = false;

    if (NetworkAvailable())

    {

     loginSuccessful= LogUserOn();

    }

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

  7. Andrew Hare says:

    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.

  8. Keith Patrick says:

    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;

  9. Robert Davis says:

    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.

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

  11. MarcelDevG says:

    Although I D want to RY,

    with the first it easier to put a breakpoint on…

  12. Jay says:

    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.

  13. Pavel Minaev says:

    > 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()

         …

      }

  14. Olivier Leclant says:

    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.

  15. David Nelson says:

    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.

  16. Scott Slack says:

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

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

  17. Greg D says:

    @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.

  18. Greg says:

    { 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.

  19. RichB says:

    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.

  20. pete.d says:

    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.  🙁

  21. Steve says:

    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…

  22. Don B says:

    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();

    }

  23. Laura T. says:

    > 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() )… }

  24. James Hart says:

    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.

  25. Jayson says:

    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.

  26. Chris B says:

    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.

  27. Tom says:

    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.

  28. 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.

  29. Greg says:

    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.

  30. Pavel Minaev [MSFT] says:

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

  31. Joost Morsink says:

    I think it really boils down to a discussion which style ‘feels’ better:

    1. use && if you like the expression style

    2. use if if you like the statement style

    of course we can interpret that as the following:

    1. use && if you like functional style

    2. use if if you like imperative style

    I like functional style, and I’m trying to convince some colleagues of its (imho anyway :)) superiority.

    I even like functional style that much that I’m willing to ignore the side effects some method call has to have the containing code construct look and/or be more functional.

    I also think short circuit boolean operators should be basic knowledge for any C# programmer

    -> if (s==null || s.Length==0) is code that would throw an exception if || wasn’t short circuited, and you really don’t want to nest an if here!

  32. Dave Sexton says:

    I think style follows semantics /and usage/.  We try to design APIs in terms of how they will be consumed, so why not imperative code as well?

    In other words, what happens after this code should be a factor in how it is written.  Here’s an example:

    bool loginSuccessful;

    if (NetworkAvailable())

     loginSuccessful= LogUserOn();

    else

     loginSuccessful= false;

    ReportSuccessToUser(loginSuccessful);

    vs.

    ReportSuccessToUser(NetworkAvailable() && LogUserOn());

    The side-effects for LogUserOn seem clearer to me now than without the call to ReportSuccessToUser.  It’s as if this new method implies that side-effects will occur – because we care about whether they were successful.

  33. Dave Sexton says:

    I want to qualify my last comment by restating "style follows semantics and usage" as "style follows semantics and style".  This is nicer because it alludes to the viral effect that code has on other code.

  34. TC says:

    Here’s a good one: a pseudocode "and" keyword being expected to short-circuit !!  :  http://en.wikipedia.org/wiki/Talk:Insertion_sort#Pseudocode_problem

  35. nog says:

    In terms of code readability a method named LogUserOn() shouldn’t return a bool.  

  36. Wes says:

    Upon reading this I immediately searched for CQS in the comments and am glad to see it show up.  Seeing as the state of being "logged in" to a system is something that can change and is what we are really modeling here, it totally makes sense to call a logon procedure and then check if the login was sucessful via a query routine.  

    CQS leads to reuse as I imagine we wouldn’t want to attempt logging in a second time just to check if the user is authenticated somewhere else.  Also, I think it would be beneficial to know if a user is no longer logged on without logging them in, so we would have to add another query routine anyways, so we might as well make LogOn void to avoid duplication.

    I guess what we come back to the semantics of separating commands from queries, in which case the separated semantics lead to a more _readable_ style(syntax) where Foo() && Bar() can always be compacted to one line, thanks to CQS.

  37. I mostly agree with James Hart. A method should do what its name implies, and return its result (if any). If it can’t do what its name implies, it should report it as an exception.

    So that leaves two options (for this example):

    1) LonOnUser(…) always returns an IPrincipal. You can call .Identy.IsAuthenticated to test whether the principal was authenticated.

    2) LogOnUser() returns an IPrincipal where .Identity.IsAuthenticated is always thrue, and throws if it can’t do that.

    Note that the method would need a rename in both cases.

    Both approaches make the original question pointless.

    Summarry: a method that returns a boolean is a function meant to compute the boolean. A method meant to do something else ("called for its side effect") should not return a boolean to indicate success or failure.

    (The only exception to this is bool methods with out parameters named TryX, such as TryParse methods.) Consequently, if functions return boolean, they can be combined with the && operator with no maintainability problem at all.

  38. Filini says:

    I tend to agree with the idea that communicating the semantics of the program fragment is important, and trying to "compress" 2 lines into 1, just because you prefer that style, risks to hide the flow you are writing.

    If we talk about the "real-word" scenario of login, then I suppose the requirements of that code might be something like: "create a method which logs a user in the system, if the network is available, and returns the status of the login".

    What if some other action needs to be taken, when the connection is not available?

    Let’s change the requirements to: "create a method which logs a user in the system, if the network is available. If the network is not available, check your client for a cached profile, to work off-line. the method returns the status of the login (real or cached)".

    Then our code would become:

    bool result;

    if( NetworkAvailable() )

        result = LogInUser();

    else

        result = ExistsCachedProfile();

    How would you change it?

    bool result = ( NetworkAvailable() ) ? LogInUser() : ExistsCachedProfile() ;

    or

    bool result = ( NetworkAvailable() && LogInUser() ) || ExistsCachedProfile() ;

    For me, the second "style" is not so nice/readable anymore.

    It all depends on what your methods do (check a value / modify the system), as Eric states in the first paragraph. The two fragments:

    bool result = NetworkAvailable() && LogInUser();

    bool result = NetworkAvailable() && IsLoggedUser();

    are the same code fragment, but they clearly perform different algorithms (the first one modifies the system). This might become misleading, for the code maintenance.

  39. rousso says:

    I always prefer the shortest way to express something in code. So I tend to prefer skipping the control flow statements if I can do the exact same thing with a direct assignment.

    Nevertheless I see other semantic issues in the examples you are presenting here, which would make the argument quite different had they been addressed:

    I would implement NetworkAvailable() method most likely as a NetworkAvailable property (because availability is a property of a network that indicates its status). I also would test the property inside LogOnUser() method which would return false if network was unavailable or even better it should throw an exception (because lack of network availability is an exceptional situation).

    To be more precise if I needed to express these things in code I would structure the whole thing to end up looking like:

    if (!Network.IsAvailable)

        throw new Network.UnavailableException();

    return User.Logon();

    or (if i didn’t want to throw an exception)

    return Network.IsAvailable ? User.TryLogOn() : false;

    which is shorter than the control flow statement but still expresses the notion that attempting a login while network is unavailable is pointless. Also naming the LogOn method as TryLogOn() denotes that the method is not going to throw an exception.

    So bottom line i would go for the shortest expressive statement foobar = foo() && bar(); but I would also take under consideration the semantics of foo() and bar() and and go for the shortest && least cryptic code by utilizing programming constructs that suite best the nature of the expressed elements.

  40. Allon Guralnek says:

    Kris really nailed it in my opinion, but I don’t agree that both approaches make the original question pointless, rather that they make the LogUserOn example pointless.

  41. RobS says:

    C# is a high-level language and should be about readability rather than brevity.

    Conversely, being overly verbose is just as bad a writing cryptic code.

    In this case, virtually nothing is gained by combining these lines and you lose a little bit of readability.

    In these cases, I would always keep them as separate cases or, as mentioned, assign the results to separate variables then compare in an IF statement

    BTW I like the variable names in helping to make self-documenting code.  When lines get too cryptic, you tend to lose that too.

  42. Bent Rasmussen says:

    I observed a colleague doing a side-effecting expression a week or so ago and tried to explain the command-query separation principle and how he could make more explicit that there was a side-effect by not chaining side-effecting calls together in an expression as if the expression really was pure. It was a pretty tough sell…

  43. Pete Wilson says:

    Personally I don’t like seeing tests or assignments of literal true/false values when it can be avoided.

    I’d probably do

    bool success = NetworkAvailable();

    if (success)

      success = LogUserOn();

  44. Not Oliver says:

    Oliver: "I used to think "shorter is always better", but using breakpoints in VS made me change my mind."

    I still think that in general "shorter is always better" (at least for simplicity in terms of semantic elements, not characters).  Using breakpoints in VS made me convinced that VS doesn't do breakpoints very well.  🙂

Skip to main content