Find the First Spaghetti

Refactoring code - to a deeper extent than what seems pragmatic at first - is a great exercise 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, shifting to a more functional style has better chances. Although there are plenty of Java 8 guides that showcase collection stream stunts, it's easy to miss good 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 results in likelihood for bugs being lower. The problem with verbosity specifically can be addressed. The following example makes the code read like an English sentence - a reasonable goal in general:

private Transactional readTransactionMetadata(MethodInvocation methodInvocation) {
        return FindFirst
                .fromLazy(
                        () -> methodInvocation.getMethod().getAnnotation(Transactional.class),
                        () -> methodInvocation.getThis().getClass().getAnnotation(Transactional.class),
                        () -> Internal.class.getAnnotation(Transactional.class)
                ).whichIsNotNull()
                .get();
}

Note that the same pattern can be relevant for a number of things, even higher level functionality like looking for an element in a cache, then - if not found there - fetching it from some webservice, etc. A possible implementation of a reusable FindFirst used in the previous sample, supporting inputs as varargs, iterables, eager or lazy elements, arbitrary find criteria, optional result, etc.

public class FindFirst<T> {
    private static final Predicate notNull = e -> e != null;
    private static final Predicate<Optional> isPresent = Optional::isPresent;

    private Optional<Iterable<T>> elements;
    private Optional<Iterable<Supplier<T>>> elementsLazy;

    public FindFirst(
            Optional<Iterable<T>> elements,
            Optional<Iterable<Supplier<T>>> elementsLazy
    ) {
        this.elements = elements;
        this.elementsLazy = elementsLazy;
    }

    @SafeVarargs
    public static <T> FindFirst<T> from(T... elements) {
        return from(Arrays.asList(elements));
    }

    public static <T> FindFirst<T> from(Iterable<T> elements) {
        return new FindFirst<>(Optional.of(elements), Optional.empty());
    }

    @SafeVarargs
    public static <T> FindFirst<T> fromLazy(Supplier<T>... suppliers) {
        return fromLazy(Arrays.asList(suppliers));
    }

    public static <T> FindFirst<T> fromLazy(Iterable<Supplier<T>> suppliers) {
        return new FindFirst<>(Optional.empty(), Optional.of(suppliers));
    }

    public Optional<T> which(Predicate<T> predicate) {
        return stream().filter(predicate).findFirst();
    }

    @SuppressWarnings("unchecked")
    public Optional<T> whichIsNotNull() {
        return which(notNull);
    }

    @SuppressWarnings("unchecked")
    public Optional<T> whichIsPresent() {
        return which((Predicate) isPresent);
    }

    private Stream<T> stream() {
        return elements.isPresent() ?
                iterableStream(elements.get()) :
                iterableStream(elementsLazy.get()).map(Supplier::get);
    }

    private static <T> Stream<T> iterableStream(Iterable<T> iterable) {
        return StreamSupport.stream(iterable.spliterator(), false);
    }
}

Performance Impact

Surprisingly, the FindFirst 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 got 10% better performance than the FindFirst version, which is what I expected due to the extra overhead FindFirst code involves. This is in the same ballpark and - except for the most extreme scenarios - will not be a bottleneck in practice.

It's interesting to point out that the functional refactoring greatly increased performance initially, since it's already optimized due to its declarative nature. The original imperative code needed intentional optimization to reach and somewhat surpass its performance.