I 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 surprisingly 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 no heavy changes by any means, but the difference in the quality of the resulting code is significant.