Wednesday, September 22, 2010

Boolean to boolean converters

Stumbled upon the following piece of code today:
if (checkTimeExpired(order.getCreated(), min)) {
 if (mailReports != null && 
  mailReports.contains(order.getSessionId())) {
  return false;
 } else {
  return true;
    }
} else {
 return false;
}
It does something simple but looks ugly and obscure. Half of the code actually deals with converting boolean values to boolean which is unnecessary. A lot of improvement can be made just by removing that:
return checkTimeExpired(order.getCreated(), min) && 
 (mailReports == null || !mailReports.contains(order.getSessionId())
This anti-pattern is suprisingly common, it is difficult to tell why. In this specific case it may have been used to break down a more complex logical statement to multiple simple ones at the price of a more complicated program structure. However the same goal can be achieved without drawbacks by refactoring, like Method Extraction:
return checkTimeExpired(order.getCreated(), min) && 
 hasSession(mailReports, order.getSessionId()); 
And possibly Move Method (if suitable):
return checkTimeExpired(order.getCreated(), min) 
 && mailReports.hasSession(order.getSessionId())

These are not heavy changes by any means, but the difference in the quality of the resulting code is significant.

No comments:

Post a Comment