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.

Replacing many if statements

I recently was asked by a colleague how to get rid of a staggering amount of if statements.

They all seem to have remarkable similarities, as is obvious from the code below.

So I initially thought about changing it to:

Of course it is also possible to create a static Map, that contains Functions. That way, the ValidationMessage instances are only created once. It depends on your needs, really.

In the end, I don't know if the improvement is really that huge.

It's closer together at least, but I feel it could be improved.

Refactoring into Streams

So, as often happens, I encountered the following code at work:

It's basically a zipped file, containing other zipped files, containing supermarket sales item data. This little bit of code just decides if a file should be parsed.

SonarLint quite rightly complained "Refactor this method to reduce its Cognitive Complexity".

And I quite quickly came up with the thought that it's really a list, where we can stop evaluating the list after the first match.

So, it becomes something like:

But, of course we have streams now:

It looks better, no?

It's a trivial example, but I like it.

Addendum

A colleague mentioned that I should do a bit of this differently. Move some more stuff into the enum.

The result becomes something like this:

Moving knowledge into the Enum, seems to be a better fit here.

On Rewriting Software

I came across the reference¹ below on "rewriting" software, and it struck a cord with me.

Turns out that, as is so often the case, the fact where "you should never ever do that"² has its successful exceptions.

References

[1] Lessons from 6 software rewrite stories

https://medium.com/@herbcaudill/lessons-from-6-software-rewrite-stories-635e4c8f7c22

[2] Joel Spolsky - Things You Should Never Do, Part I

https://www.joelonsoftware.com/2000/04/06/things-you-should-never-do-part-i/

Another weird if statement

Found the following code at my work.

if (order.getOrderCode() == null) 
{
    simpleOrder = true;
}
else 
{
    if (order.getOrderCode().length() == 0) 
    {
        simpleOrder = true;
    }
}

Refactor.

Note: There are plenty of libraries that offer very good single static operations to replace the if condition, but that's not important right now.

Addendum on Post "Decrease Indentation"

This is a small addendum to my post on Decrease Identation

I noticed that the same thing can be done in for-loops and the like, by means of continue. I just don't know if it's in common use at the moment.

Bad example:

/**
 * Process the proper states.
 */
public void processStates()
{
    List<State> stateList = new ArrayList<State>();
   
    for (State state : stateList) 
    {
        if (!state.equals(State.INVALID))
        {
            Capital capital = State.getCapital();
            if (capital != null && capital.size() > 1000000)
            {
                stateList.add(state);
            }
        }
    }
    stateService.processStates(stateList);
    refresh();
}

Good example (bad example refactored):

/**
 * Process the proper states.
 */
public void processStates()
{
    List<State> stateList = new ArrayList<State>();
   
    for (State state : stateList) 
    {
        if (state.equals(State.INVALID))
        {
            // invalid state!!!
            continue;
        }
        Capital capital = State.getCapital();
        if (capital == null || capital.size() <= 1000000)
        {
            // capital doesn't exists or not impressive enough!
            continue;
        }
        stateList.add(state);
    }
    stateService.processStates(stateList);
    refresh();
}

Decrease Indentation

And when I talk about indentation, I mean nested if statements, another one of my pet peeves.

This specific refactoring is called: Replace Nested Conditional with Guard Clauses^[1].

Preconditions (GuardClauses) are a great example of where it is okay to return early at the start of the function.

It is very convenient to prevent the ArrowAntiPattern.

Here a bad example:

public boolean getPayment(String a, Double b)
{
    boolean result = false;
    if (a != null && b != null && !a.equals("") && b >= 0.0)
    {
        User user = getUser(a);
        if (user != null && user.getAccountValue() >= b))
        {
            user.setAccountValue(user.getAccountValue() - b);
            return true;
        }
        else
        {
            log.debug("User account not found or insufficient funds.");
            result = false;
        }
    }
    else
    {
        log.debug("Bad values.");
        result = false;
    }
    return result;
}

Here a good example (bad example refactored):

public boolean getPayment(String a, Double b)
{
    // preconditions
    if (a == null || b == null)
    {
        log.debug("Null values");
        return false;
    }
    if ("".equals(a) || b <= 0.0)
    {
        log.debug("Bad values");
        return false;
    }
    User user = getUser(a);
    if (user == null)
    {
        log.debug("User account not found.");
        return false;
    }
    if (user.getAccountValue() < b)
    {
        log.debug("Insufficient funds.");
        return false;
    }
    user.setAccountValue(user.getAccountValue() - b);
    return true;
}

One important skill here is the proper negation of an expression. You'd be surprised how many people lack this skill.

References

[1] Refactoring - Improving the design of existing code

Martin Fowler