Style follows semantics

Artikel
02/01/2010

Which is better style?

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

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

Anonymous
February 01, 2010
The comment has been removed
Anonymous
February 01, 2010
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.
Anonymous
February 01, 2010
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.
Anonymous
February 01, 2010
I'm with Eric on both cases, but just throwing another option out there: bool loginSuccessful = false; if (NetworkAvailable()) { if (LogUserOn()) loginSuccessful = true; }
Anonymous
February 01, 2010
The comment has been removed
Anonymous
February 01, 2010
I'd probably go with: bool loginSuccessful = false; if (NetworkAvailable()) { loginSuccessful= LogUserOn(); } I like to lift as much out of conditionals as possible.
Anonymous
February 01, 2010
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.
Anonymous
February 01, 2010
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;
Anonymous
February 01, 2010
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.
Anonymous
February 01, 2010
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).
Anonymous
February 01, 2010
Although I D want to RY, with the first it easier to put a breakpoint on...
Anonymous
February 01, 2010
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.
Anonymous
February 01, 2010
The comment has been removed
Anonymous
February 01, 2010
The comment has been removed
Anonymous
February 01, 2010
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.
Anonymous
February 01, 2010
bool loginSuccessful = (NetworkAvailable()) ? LogUserOn() : false; Clearly indicates intention, and saves those precious code lines for future generations.
Anonymous
February 01, 2010
@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.
Anonymous
February 01, 2010
{ 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.
Anonymous
February 01, 2010
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.
Anonymous
February 01, 2010
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. :(
Anonymous
February 01, 2010
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...
Anonymous
February 01, 2010
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(); }
Anonymous
February 01, 2010
> 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() )... }
Anonymous
February 01, 2010
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.
Anonymous
February 01, 2010
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.
Anonymous
February 01, 2010
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.
Anonymous
February 01, 2010
The comment has been removed
Anonymous
February 01, 2010
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.
Anonymous
February 02, 2010
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.

Anonymous
February 02, 2010
The comment has been removed
Anonymous
February 02, 2010
I think it really boils down to a discussion which style 'feels' better:

use && if you like the expression style
use if if you like the statement style of course we can interpret that as the following:
use && if you like functional style
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!

Anonymous
February 02, 2010
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.
Anonymous
February 02, 2010
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.
Anonymous
February 02, 2010
Here's a good one: a pseudocode "and" keyword being expected to short-circuit !! : http://en.wikipedia.org/wiki/Talk:Insertion_sort#Pseudocode_problem
Anonymous
February 02, 2010
In terms of code readability a method named LogUserOn() shouldn't return a bool.
Anonymous
February 02, 2010
The comment has been removed
Anonymous
February 02, 2010
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):

LonOnUser(...) always returns an IPrincipal. You can call .Identy.IsAuthenticated to test whether the principal was authenticated.
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.

Anonymous
February 03, 2010
The comment has been removed
Anonymous
February 15, 2010
The comment has been removed
Anonymous
February 16, 2010
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.
Anonymous
February 17, 2010
The comment has been removed
Anonymous
March 07, 2010
The comment has been removed
Anonymous
April 06, 2010
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();
Anonymous
November 02, 2010
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. :-)

Freigeben über

Style follows semantics

Comments

Zusätzliche Ressourcen