Engineering Blog

We’ve all seen them.

We’ve all written them.

Long, complex methods are hard to read. If they’re hard to read, they’re obviously hard to understand. If they’re hard to understand, they’re hard to maintain. Once you throw in hard to test, it quickly becomes obvious that long complex methods are bad.

One solution for long methods is to refactor to composed methods. A quick bit of Googling suggests that the term Composed Method was coined by Kent Beck. I first heard the term in a workshop on refactoring to patterns that I attended several years ago.

So what is a composed method? The simplest definition I’ve heard is a composed method is composed only of calls to other methods – no other statements. That’s a bit of an oversimplification but it’s a good starting point. Here’s a more formal definition, lifted from the c2.com wiki:

Divide your program into methods that perform one identifiable task. Keep all of the operations in a method at the same level of abstraction. This will naturally result in programs with many small methods, each a few lines long.

That’s a good definition, but it’s a bit abstract. The way I like to think about composed methods is that methods should either be organizers or actors, never both. Organizers call other methods and may have simple control structures, like “if” or “for”. Actors encapsulate a few lines of logic or actions into a single place.

Let’s take a look at a concrete example. The following Java code is loosely based on a chunk of persistence code from Rally. I produced it by reverse applying the steps of the refactoring we’re about to discuss.

protected ModelAndView save(HttpServletRequest req, Persistable persistable) {
    Metadata metadata = metadataService.getMetadataFor(persistableClass);
    if ((metadata != null && (!metadata.isEnabled())) || !securityManager.isAuthorized(persistable)) {
        throw new TypeDisabledException();
    }
    DataBinder dataBinder = new DataBinder(persistable, BINDING_KEY);
    this.initializeBinderCommand.execute(dataBinder);
    dataBinder.bind(req);
    if(dataBinder.hasErrors()) {
        ModelAndView modelAndView = new ModelAndView(VIEW);
        modelAndView.addObject(ERROR_CODE, BINDING_FAILED);
        return modelAndView;
    }
    try {
        persistenceService.commitChangesTo(persistable);
    } catch (ValidationException e) {
        ModelAndView modelAndView = new ModelAndView(VIEW);
        modelAndView.addObject(ERROR_CODE, VALIDATION_FAILED);
        return modelAndView;
    }
    ModelAndView modelAndView = new ModelAndView(VIEW);
    modelAndView.addObject(EDIT_ITEM, persistable);
    return modelAndView;
}

There are a number of issues with this code. Rampant duplication. Lots of conditional logic. Complex conditional statements. The biggest problem to me is that you can’t read it and immediately know what it’s supposed to be doing.

Luckily, refactoring to a composed method is pretty easy to do in most modern IDEs. IDEs like IntelliJ IDEA and Eclipse provide safe support for extracting methods and variables. Let’s take a look at what the method looks like after a little IntelliJ-aided love (actually, this refactoring was done by hand while writing the blog post, so please excuse any typos).

protected ModelAndView save(Request req, Persistable persistable) {
    authorizeOperation(persistable);
    BindingResult bindingResult = bindRequest(req, persistable);
    if (bindingResult.hasErrors()) {
        return createErrorView(BINDING_FAILED);
    }
    try {
        saveAndCreateSuccessView(persistable);
    } catch (ValidationException e) {
        return createErrorView(VALIDATION_FAILED);
    }
}

private void authorizeOperation(Persistable persistable) {
    if (typeNotSupportedForCustomer() || userNotAuthorizedToEdit(persistable)) {
        throw new TypeDisabledException();
    }
}

private boolean typeNotSupportedForCustomer() {
    Metadata metadata = metadataService.getMetadataFor(persistableClass);
    boolean typeHasMetadata = metadata != null;
    boolean typeDisabledForCustomer = !metadata.isEnabled();
    return typeHasMetadata && typeDisabledForCustomer;
}

private boolean userNotAuthorizedToEdit(Persistable persistable) {
    return !securityManager.isAuthorized(persistable);
}

private BindingResult bindRequest(Request req, Persistable persistable) {
    DataBinder dataBinder = new DataBinder(persistable, BINDING_KEY);
    this.initializeBinderCommand.execute(dataBinder);
    return dataBinder.bind(req);
}

private ModelAndView createErrorView(String errorCode) {
    ModelAndView modelAndView = new ModelAndView(VIEW);
    modelAndView.addObject(ERROR_CODE, errorCode);
    return modelAndView;
}

private ModelAndView saveAndCreateSuccessView(Persistable persistable) {
    saveChanges(persistable);
    return createSuccessView(persistable);
}

private void saveChanges(Persistable persistable) {
    persistenceService.commitChangesTo(persistable);
}

private void createSuccessView(Persistable persistable) {
    ModelAndView modelAndView = new ModelAndView(VIEW);
    modelAndView.addObject(EDIT_ITEM, persistable);
    return modelAndView;
}

The save method can now be read and understood without much thought, because it’s composed of nothing but calls to other well-named methods and very simple control statements. It’s one of the organizing methods I talked about earlier. Also, we’ve removed some duplication by extracting createErrorView. There are more lines of code in total, but the overall readability is improved significantly.

Organizing methods tend to be the most readable, but even some of the lines that now constitute actor methods are improved. Take a look at typeNotSupportedForCustomer. We’ve introduced a series of variables that are also well-named and clearly communicate what the otherwise opaque conditional is testing for.

This method looks a lot better now, and the refactoring has produced several smaller methods that could be tested in isolation. This is a starting point, not the end solution. Now that much of the code is broken out into smaller pieces, some of the methods look like good candidates to be classes in their own right, but that’s a topic for another post… ErrorView anyone?