No, this is not bad practice. Relying on short-circuiting of conditionals is a widely accepted, useful technique--as long as you are using a language that guarantees this behavior (which includes the vast majority of modern languages).
Your code example is quite clear and, indeed, that is often the best way to write it. Alternatives (such as nested if
statements) would be messier, more complicated, and therefore harder to understand.
A couple of caveats:
Use common sense regarding complexity and readability
The following is not such a good idea:
if ((text_string != null && replace(text_string,"$")) || (text_string = get_new_string()) != null)
Like many "clever" ways of reducing the lines in your code, over-reliance on this technique can result in hard-to-understand, error-prone code.
If you need to handle errors, there is often a better way
Your example is fine as long as it is a normal program state for smartphone to be null. If, however, this is an error, you should incorporate smarter error handling.
Avoid repeated checks of the same condition
As Neil points out, many repeated checks of the same variable in different conditionals are most likely evidence of a design flaw. If you have ten different statements sprinkled through your code that should not be run if smartphone
is null
, consider whether there is a better way to handle this than checking the variable each time.
This isn't really about short-circuiting specifically; it's a more general problem of repetitive code. However, it is worth mentioning, because it is quite common to see code that has many repeated statements like your example statement.
If the e2
catch is, as you say, only to catch errors in initializing x
and doing otherStuff
you could extract it to a seperate method. This also seperates the logic nicely and allows you to potentially give a meaningful name to otherStuff
.
public X Foo() {
try{
X x = blah;
otherStuff;
return x;
}
catch(Exceptions2 e2){
...
}
}
public void Bar() {
X x = Foo();
for (int i = 0; i < N; i++){
try{
String y = Integer.toString(i);
f(x);
}
catch(Exceptions1 e1){
System.err.println(... + y + ...);
System.exit(0);
}
}
}
Best Answer
I think the second form is fine, and also more clearly represents what you're trying to do. You say that...
It doesn't matter if all languages do that. You're writing in one particular language, so it only matters if that language does that. Otherwise, you're essentially saying that you shouldn't use the features of a particular language because other languages might not support those features, and that's just silly.