Following on from the previous article in the series, another "programming in the small" style thing that I often see, even by people who understand non-interacting fermions, are conditionals that are more complicated than they need to be.
Consider the following (Java) code:
public boolean isWhatever() {
if(someCondition() == true){
return true;
}else{
return false;
}
}
Must control fist of death.
someCondition() == true can be replaced by someCondition(), which is easier to read.
There's something about the existence of the "true" which I think sends my brain (and I guess others) thinking about programming rather than the meaning of the code.
For the opposite value of the condition, some people do someCondition() == false rather than !someCondition() which I have heard justified on the basis that ! is easy to miss - which is a reasonable point but again I find obscures the meaning (if you read the code rather than interpret it as a program). It's not: "if the water to make my tea is boiling hot is false then throw a hissy fit" - no - it's "if the water to make my tea is not boiling hot then throw a hissy fit".
In python the equivalent of ! is not, which I think is much more preferable - but unfortunately my programming in the small articles don't result in changes to languages quite yet. I think ! is typical of the wrong-headed "C" based nonsense that also includes each case of a switch statement needing a break - but maybe that should be another article.
Anyway, back to the plot. This leaves you with:
public boolean isWhatever() {
if(someCondition()){
return true;
}else{
return false;
}
}
Must still control fist of death.
This can be replaced by:
public boolean isWhatever() {
return someCondition();
}
less is most definitely more. I've also seen someCondition() ? true : false, which is also pointless.
David Peterson suggested another category of over complicated conditional (thanks David) - where the conditional is negatively named, for example:
if (!isTeaNotHot() && isCupNotEmpty()) {
bewareOfSpillage();
}
is much easier to read with positively named methods:
if (isTeaHot() && !isCupEmpty()) {
bewareOfSpillage();
}
Other conditional related stuff that annoys me (but less obviously - I probably won't spill my tea when faced with such code) is where there are more if statements than necessary, that could be replaced by judicious use of &&, || and ==.
Consider the following code:
public boolean isWhatever() {
if (someCondition()) {
if (someOtherCondition()) {
return true;
} else if (yetAnotherCondition()) {
return true;
}
}
return false;
}
this can be replaced by:
public boolean isWhatever() {
return someCondition() && (someOtherCondition() || yetAnotherCondition());
}
which is so much easier to understand (given someCondition(), someOtherCondition() and yetAnotherCondition() mean something). In general, I think fewer if statements lead to a more easily understood meaning.
Something based on my current reality:
public boolean successfullJobInterview(boolean candidateIsSuitable, boolean candidateIsOfferedJob) {
if (!candidateIsSuitable && candidateIsOfferedJob) {
return false;
}
if (candidateIsSuitable && !candidateIsOfferedJob) {
return false;
}
return true;
}
it looks reasonably logical and simple - how could this be improved?
public boolean successfullJobInterview(boolean candidateIsSuitable, boolean candidateIsOfferedJob) {
return candidateIsSuitable == candidateIsOfferedJob;
}
which is simpler if you think about it.
Posted by ivan at August 6, 2006 9:02 AMHi Ivan,
I agree! Thomas Timbul and I worked on a tool to identify these sorts of patterns, mark them in your IDE and provided a quick fix automatic refactoring to simplify. Unfortunately we only did this for the teaching language Kenya rather than full Java, but the idea was that you'd provide this facility for students when they were learning to program, and then later in life they would do the right thing.
We presented this as the ESEC/FSE conference last year. The paper is at http://chatley.com/articles/kenya-eclipse-paper.pdf
--Robert
Posted by: Robert Chatley at August 6, 2006 10:43 AMI think much of this is left over from the days of C/C++ when there wasn't a boolean type and people had to be a little more careful. Where 0 is false and not 0 is true, then people often had to code defensivly to be sure. Now most of us are supposed to live in a better world, but the habit remains.
Oh, and sometimes people stop before they're really, really done.
Posted by: Steve Freeman at August 6, 2006 1:56 PMFYI: The latest versions of C/C++ standards (implemented by gcc) allow using "not" instead of "!", and "and" instead of "&&" and "or" instead of "||". Much more readable.
Posted by: keith ray at August 6, 2006 2:10 PMTotally agree! The cost of thinking what does the code do by human is higher than thinking what does the code do by computer. We should care more about human, instead of computer. i.e the logical correct is important, but not the only important thing.
Posted by: taowen at August 7, 2006 1:59 AMReminds me of the new operator in VB.NET 2.0 that everyone got all fussy about- IsNot.
Old Syntax:
If Not Foo Is AnotherFoo
...
End If
New Syntax:
If Foo IsNot AnotherFoo
...
End If
I find the new operator far easier to read.
Posted by: Chris Shain at August 7, 2006 9:10 PMIf you think about it some more ;)
For the last example:
public boolean successfullJobInterview(boolean candidateIsSuitable, boolean candidateIsOfferedJob) {
return candidateIsSuitable == candidateIsOfferedJob;
}
I think a better way would be
return candidateIsSuitable && candidateIsOfferedJob;
if candidateIsSuitable AND candidateIsOfferedJob then successfulJobInterview makes more sense than candidateIsSuitable EQUALS candidateIsOfferedJob then successfulJobInterview.
Posted by: Avid Coder at August 9, 2006 12:55 AMHello Avid Coder,
"candidateIsSuitable && candidateIsOfferedJob" isn't the condition I am looking for. A job interview is also successful if the candidate isn't suitable and isn't offered the job (as far as I am concerned and as far as the code I'm refactoring in this example is concerned).
Ivan
Posted by: Ivan at August 9, 2006 5:57 AMHmm...any thoughts about how these optimizations survive the wrapper-ing of little-b boolean to big-B Boolean? Since big-B Boolean is trinary (true, false, null), a refactoring that removes the Boolean.TRUE here changes the semantics:
if (flag == Boolean.TRUE) {
// Boolean.TRUE
} else {
// Boolean.FALSE or null
}
In order to remove the "== Boolean.TRUE" from the expression, you'd have to either reverse the logic:
if (flag == null || !flag) {
// null or Boolean.FALSE
} else {
// Boolean.TRUE
}
Or catch an exception:
try {
if (flag) {
// Boolean.TRUE
} else {
// Boolean.FALSE
}
} catch (NullPointerException npe) {
// null
}
Or, maybe the moral of the story is that you need be extra careful if you do a refactoring where you wrap a primitives?
Any thoughts?
Posted by: Mick Killianey at August 31, 2006 1:26 PMHi Mickey,
an interesting point - I hadn't considered it.
I guess I tend to use the primitive type as much as possible - why would I want to return a B rather than a b? If the conditional my code was checking (in the first example in the article) was a B rather than a b, then I'd use:
public boolean isWhatever() {
return Boolean.TRUE == someCondition();
}
to avoid the null check and if statement. Indeed, you are right that the B has to be treated carefully in case of having a null value.
Ivan
Posted by: ivan at August 31, 2006 8:46 PMEven then how your condition is gonna work? The interview can be a successful even if both conditions are not equal. Like candidate is suitable but not get the offer. I think you need not make decision based on any of those condition. Not sure about your case, though.
About the "cup of tea" condition. It is more or less fine to me. But I would rather check for emplyCup first. It sounds more logical to me and a real life thing.
Posted by: Adeel Ansari at September 7, 2006 3:50 AMOops!! Sorry, regarding successful interview example. I found that you just refactored the given code. Its fine. My mistake.
Thanks.
Posted by: Adeel Ansari at September 7, 2006 3:54 AM