Functional Refactoring

One of the biggest challenges most software engineers face is, to maintain a badly written code. Refactoring code is a very difficult task, which can break a lot of things in a so called working software. There are only two choices one can make, either add more mess to the code or dig deep into the dirt and try to clean it. Most of the times we follow a mixed approach which is most pragmatic.

In this article we will see some functional approaches to refactor code.

Here we have simple entities Customer and Order –

public class Customer {
    int customerId;
    int age;
    String name;
    List<Order> orders;
}

 

class Order {
    int orderId;
    int numberOfItems;
    int customerId;
}

There is a method to find a customer matching an order id. Take a look at the code below, what problems do you see ?

public static Customer findCustomerByOrder(int orderId) {
    List<Customer> customers = getAllCustomers();
    for(Customer c : customers) {
        List<Order> orders = c.orders;
        for(Order o : orders) {
            if(o.orderId == orderId) {
                return c;
            }
        }
    }
    return null;
}

As you guessed right its a pretty old style iteration logic which has mutable state for customer, order and also it returns null which may result in NullPointerException if not handled on the caller side.

What’s the problem with mutable state ? 

In general mutable state is a big problem in multithreaded environment, but here the problem is that the programmer has to unnecessarily create references to objects which is not necessary and can be handled in a very sophisticated manner. Most languages support streaming which hides the iteration logic from the programmer.

Take a look at a better version below :

public static Optional<Customer> findCustomerByOrder(final int orderId) {
    return getAllCustomers()
            .stream()
            .filter(c -> c.orders.stream()
                      .anyMatch(o -> o.orderId == orderId))
            .findFirst();
}

A very easy improvement was to hand over the iteration to the language constructs and focus only on the business logic. This is an approach inspired by the functional programming paradigm. You don’t have to know how iteration is taking place, you just care about the functions and the parameters to pass.

Another improvement is that, it returns an Optional which informs to the caller that the value may not be present always. So a more higher level checks can be added by the caller than just checking for nulls.

If you see closely, filter is taking a lambda as an input. Lambda’s are a great way of representing functions with lesser code :).

Lets take a closer look at the lambda :

c -> c.orders.stream().anyMatch(o -> o.orderId == orderId)

There is a sub lambda within which can be broken down again as :

o -> o.orderId == orderId

Basically if you imagine the left part of the lambda as the input and the right part as the function body it can be written as follows :

boolean matchOrderId(Order o, int orderId) {
    return o.orderId == orderId;
}

Also for the customer lambda :

public boolean matchOrderId(Customer c, int orderId) {
    return c.orders.stream().anyMatch(o -> matchOrderId(o, orderId));
}

Can we improve further ? I think Yes!

Let’s see another function :

public static Optional<Customer> findACustomerOlderThan(final int age) {
    return getAllCustomers()
            .stream()
            .filter(c -> c.age > age)
            .findFirst();
}

And another function :

public static Optional<Customer> findACustomerWithName(final String name) {
    return getAllCustomers()
            .stream()
            .filter(c -> Objects.equals(c.name, name))
            .findFirst();
}

Do you see the problem ?

One problem is that we are passing a parameter name, orderId or age here and using it as part of predicate logic. It is again a mutable state, of course we can handle it but its still a code smell.

Another problem is that, in all these three functions, we are iterating over the list of customers and finding a customer matching certain criteria. Except one line everything is repeated. So for all other matching criteria to find a customer you will end up writing many functions.

Solution : As we said only one line is different in each function, what is this line? It is a condition which results into true or false. It is a function which given a customer returns true or false for a certain matcher. Can we take this function/predicate out and reuse the rest of it ? Yes! Lets do it.

Optional<Customer> findACustomerMatchingPredicate(
        Predicate<Customer> customerPredicate) {
    return getAllCustomers()
            .stream()
            .filter(customerPredicate)
            .findFirst();
}

//Calling for separate matchers using lambda's
findACustomerMatchingPredicate(c -> c.age > 30);
findACustomerMatchingPredicate(c -> Objects.equals(c.name, "Messy"));
findACustomerMatchingPredicate(c -> c.orders.stream().anyMatch(o -> o.orderId == 123));

If you see above we are now passing the predicate as a parameter to the function. This is a pure higher order function, which takes a function as parameter and does not have any side effect. It is very safe and least error prone.

Using this approach a lot of redundant code can be reduced to a very compact and sophisticated code.

I hope you like the article, share your feedback.

Cheers, 

Kaivalya