So, I saw some code that I didn't like, and I decided to refactor it.
The code was thusly.
public boolean onLeave() { boolean valid = super.onLeave(); Account account = database.getAccount(); ShoppingList shoppingList = database.getShoppinglist(account); if (valid) { valid = !shoppingList.isEmpty(); } if (valid) { valid = !blockedAccounts.contains(account); } if (valid) { String country = account.getPerson().country(); valid = webshop.alsoShipsTo(country); } if (valid) { valid = account.hasCreditcardAttached() || account.hasPrepaidcardAttached() || account.hasCash(); } return valid; }
And I thought to myself, it's really just a list of expressions, where each one is evaluated until you find one that evaluates to false, and then you stop.
So, that's basically a Stream where you filter out all the "false" values, and get the first one.
So, I decided to refactor it just like that.
public boolean onLeave() { Account account = database.getAccount(); ShoppingList shoppingList = database.getShoppinglist(account); List<Supplier<Boolean>> checks = new ArrayList<>(); checks.add(super::onLeave); checks.add(() -> !shoppingList.isEmpty()); checks.add(() -> !blockedAccounts.contains(account)); checks.add( () -> { String country = account.getPerson().country(); return webshop.alsoShipsTo(country); }); checks.add(() -> account.hasCreditcardAttached() || account.hasPrepaidcardAttached() || account.hasCash()); return checks.stream() .filter(t -> t.get().equals(Boolean.FALSE)) .findFirst() .isEmpty(); }
I proudly showed this to my colleagues, and they immediately shook their heads in disgust.
Apparently, in my eagerness to use Lambdas and Streams and all that cool stuff, what I really had done is recreated the short-circuit version of an If statement in Streams.
I find myself turning to Lambdas and Streams when in reality these are not needed, and my eventual solution works fine without them.
So, rewriting this as a short-circuited IF statement looks like this:
public boolean onLeave() { Account account = database.getAccount(); ShoppingList shoppingList = database.getShoppinglist(account); return super.onLeave() && !shoppingList.isEmpty() && !blockedAccounts.contains(account) && webshop.alsoShipsTo(account.getPerson().country()) && (account.hasCreditcardAttached() || account.hasPrepaidcardAttached() || account.hasCash()); }
Granted, a few more and my IntelliJ will start to complain about the number of expressions in the if statement, and it's possible to clean it up a little by creating separate methods for some of the expressions. But I feel it looks fine.
So, in conclusion, just because you know something cool and shiny, it's no reason to try and use it everywhere!
It is a corrolary that in order to properly use something, you must have some knowledge or experience of when and where it's best to be used.
