Fabulous Adventures In Coding
Eric Lippert is a principal developer on the C# compiler team. Learn more about Eric.
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;
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;
bool canUseCloud = NetworkAvailable() && UserHasFreeSpaceInCloud();
I would always choose the latter; here we’re using && idiomatically to compute a value safely.
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!
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:
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.
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.
Here's a good one: a pseudocode "and" keyword being expected to short-circuit !! : http://en.wikipedia.org/wiki/Talk:Insertion_sort#Pseudocode_problem
In terms of code readability a method named LogUserOn() shouldn't return a bool.
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.
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.
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:
if( NetworkAvailable() )
result = LogInUser();
result = ExistsCachedProfile();
How would you change it?
bool result = ( NetworkAvailable() ) ? LogInUser() : ExistsCachedProfile() ;
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.
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:
throw new Network.UnavailableException();
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.
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.
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.
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...
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();
success = LogUserOn();
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. :-)