Wednesday, February 11, 2015

Find the first spaghetti

Refactoring code - to a deeper extent than what seems pragmatic at first - is a great excercise to learn and shape coding style. In this post I'm gonna take a look at some code taken from Google Guice and see how some functional refactoring can improve on it. This is not an if you see this, do that kind of guide, rather exploring some possibilities and alternatives. The same thought pattern may contribute to solving other problems too.

The following code is part of an interceptor that manages annotation-driven transactions. It looks for a @Transactional annotation on the target method, if it can't find one looks for one on the class, if it can't find one there either, uses some default.
public class JpaLocalTxnInterceptor {
    // ...
    private Transactional readTransactionMetadata(MethodInvocation methodInvocation) {
        Transactional transactional;
        Method method = methodInvocation.getMethod();
        Class targetClass = methodInvocation.getThis().getClass();

        transactional = method.getAnnotation(Transactional.class);
        if (null == transactional) {
            // If none on method, try the class.
            transactional = targetClass.getAnnotation(Transactional.class);
        }
        if (null == transactional) {
            // If there is no transactional annotation present, use the default
            transactional = Internal.class.getAnnotation(Transactional.class);
        }

        return transactional;
    }

    @Transactional
    private static class Internal {
        private Internal() {
        }
    }
    // ...
}
For illustration, a possible service this interceptor can be applied to:
@Transactional
public class Service {
    public void foo() {
        
    }

    @Transactional(rollbackOn = {FileNotFoundException.class})
    public void bar() {

    }
}

Functional alternative

Yoda conditions aside, readTransactionMetadata() is not that great to read: reference re-assignments, high cyclomatic complexity, etc. "Conventional" refactoring steps like method extraction wouldn't help much, more functional style has better chances. It's easy to miss functional opportunities when the starting point is not a collection of elements, but complex imperative code.

Notice that the code can be interpreted as a simple "find first" logic. There is an ordered collection of elements (@Transactional of method, @Transactional of class, default @Transactional) and we're looking for the first one based on some criteria (not null). The original imperative code can be transformed to a single statement:
private Transactional readTransactionMetadata(MethodInvocation methodInvocation) {
        return Arrays.asList(
                methodInvocation.getMethod().getAnnotation(Transactional.class),
                methodInvocation.getThis().getClass().getAnnotation(Transactional.class),
                Internal.class.getAnnotation(Transactional.class)
        ).stream()
                .filter(transactional -> transactional != null)
                .findFirst()
                .get();
}
The code solves the problem but there is a side-effect: value of all 3 elements are evaluated eagerly. The original imperative code does this lazily, avoiding unnecessary reflection operations. The Supplier interface (provided by JDK) is useful to express lazy evaluation:
public interface Supplier<T> {
        T get();
}
The solution that makes use of it:
private Transactional readTransactionMetadata(MethodInvocation methodInvocation) {
        return suppliers(
                () -> methodInvocation.getMethod().getAnnotation(Transactional.class),
                () -> methodInvocation.getThis().getClass().getAnnotation(Transactional.class),
                () -> Internal.class.getAnnotation(Transactional.class)
        ).stream()
                .map(Supplier::get)
                .filter(transactional-> transactional != null)
                .findFirst()
                .get();
}

@SafeVarargs
private static <T> List<Supplier<T>> suppliers(Supplier<T>... suppliers) {
        return Arrays.asList(suppliers);
}
The new code is almost equivalent to the original in terms of functionality. One could argue that it's still verbose, not simpler to read than the original and it's up to taste. There are a few advantages: it does makes the main pattern of the logic more explicit, has low cyclomatic complexity which generally leads lower likelihood for bugs.

Performance impact

Surprisingly the functional variant performed about 40% faster than the original imperative code. I needed to do some digging to find out why. It turned out the reason was determining "targetClass" in the original code too early - even in cases the class is not needed later. I optimized that and the imperative code got 10% better, which is what I expected due to the extra overhead the functional code involves. This is in the same ballpark and - except for the most extreme scenarios - will not be a bottleneck in practice. In a way the functional code is "natively optimized" due to it's declarative, lazy nature. The original imperative did need intentional optimization to reach and somewhat surpass it's performance.

No comments:

Post a Comment