Refactoring

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.