Simple trick to DRY your Grails controller

Wednesday, June 5, 2013

Grails controllers are not very DRY. It's easy to find duplicated code fragments in default generated controller. Take a look at code sample below. It is duplicated four times in show, edit, update and delete actions:

class BookController {
    def show() {
       def bookInstance = Book.get(params.id)
       if (!bookInstance) {
            flash.message = message(code: 'default.not.found.message', args: [message(code: 'book.label', default: 'Book'), params.id])
            redirect(action: "list")
            return
        }
        [bookInstance: bookInstance]
    }
}

Why is it duplicated?

There is a reason for that duplication, though. If you move this snippet to a method, it can redirect to "list" action, but it can't prevent controller from further execution. After you call redirect, response status changes to 302, but after method exits, controller still runs subsequent code.

Solution

At TouK we've implemented a simple trick to resolve that situation:

  1. wrap everything with a simple withStoppingOnRender method,
  2. whenever you want to render or redirect AND stop controller execution - throw EndRenderingException.

We call it Big Return - return from a method and return from a controller at once. Here is how it works:

class BookController {
    def show(Long id) {
        withStoppingOnRender {
            Book bookInstance = Book.get(id)
            validateInstanceExists(bookInstance)
            [bookInstance: bookInstance]
        }
    }

    protected Object withStoppingOnRender(Closure closure) {
        try {
            return closure.call()
        } catch (EndRenderingException e) {}
    }

    private void validateInstanceExists(Book instance) {
        if (!instance) {
            flash.message = message(code: 'default.not.found.message', args: [message(code: 'book.label', default: 'Book'), params.id])
            redirect(action: "list")
            throw new EndRenderingException()
        }
    }
}

class EndRenderingException extends RuntimeException {}

Example usage

For simple CRUD controllers, you can use this solution and create some BaseController class for your controllers. We use withStoppingOnRender in every controller so code doesn't look like a spaghetti, we follow DRY principle and code is self-documented. Win-win-win! Here is a more complex example:

class DealerController {
    @Transactional
    def update() {
        withStoppingOnRender {
            Dealer dealerInstance = Dealer.get(params.id)
            validateInstanceExists(dealerInstance)
            validateAccountInExternalService(dealerInstance)
            checkIfInstanceWasConcurrentlyModified(dealerInstance, params.version)
            dealerInstance.properties = params
            saveUpdatedInstance(dealerInstance)
            redirectToAfterUpdate(dealerInstance)
        }
    }
}

7 comments:

  1. This is crazy. Exceptions are very expensive (especially in Groovy) and shouldn't be used for flow control. Why not just return boolean from validateInstanceExists? At least override fillInStackTrace() in EndRenderingException as a no-op so you don't waste cycles filling in the stack frame info that you don't use.

    ReplyDelete
    Replies
    1. Thank you Burt for your hints. Maybe it's crazy but it's only way to achieve complex flows in controllers.

      I won't return boolean from each of these methods, because I don't want to check return values. It's like checking error codes in C.

      I will take a look into fillInStackTrace. I didn't know it costs so much.

      Delete
    2. Burt, as I think of this, when I throw EndRenderingException I mean it - break the flow, something has gone wrong, put user to list or back to editing.

      Delete
  2. Your are using exceptions as flow control. That is atrocious. There is nothing exceptional going on.

    ReplyDelete
    Replies
    1. Thank you for your comment. Take a look at DealerController example. If something goes wrong in any of these methods - this is exceptional. I want them to stop processing. That's what they do.

      Can you rewrite DealerController example more clearly, without EndRenderingException?

      Delete
  3. Hello Mr Kalkosiński,

    Nice blog! Is there an email address I can contact you in private?

    Thanks,

    Eleftheria Kiourtzoglou


    Head of Editorial Team

    Java Code Geeks
    email: eleftheria[dot]kiourtzoglou[at]javacodegeeks[dot]com

    ReplyDelete
  4. I know this is over a year old, but Burt and Michael are right. Using exceptions for flow control is really bad. Not wanting to check what your functions return because it's like 'checking error codes in C' is one of the most bizarre things I've ever heard. Where you've gone wrong is being overzealous in your refactoring.

    If you did have a method that returned a boolean, you'd be using an if statement in your show/update/edit/delete actions to switch based on how the if statement in your refactored method turned out. It's fairly obvious that the if block is not against DRY as it's required to return early (or not)- if you try to refactor it, you have to replicate the flow control, leading to the repetition you were trying to avoid!

    If validating your object is more complex, it makes perfect sense to extract that out into a method, but return a boolean as to whether the validation is succesful or not and use those for logical flow! If the redirect is sufficiently complex, refactor that /seperately/ and call it inside the if block, followed by the break-out return or just allow the else block to exclude the rest of the code (it looks like you're already haflway there with the redirectToAfterUpdate method...). That means your code should look something like this:

    class DealerController {
    @Transactional
    def update() {
    Dealer dealerInstance = Dealer.get(params.id)
    if (!validateInstanceExists(dealerInstance) || !validateAccountInExternalService(dealerInstance) ||
    ifInstanceWasConcurrentlyModified(dealerInstance, params.version)) {
    redirectToList()
    } else {
    dealerInstance.properties = params
    saveUpdatedInstance(dealerInstance)
    redirectToAfterUpdate(dealerInstance)
    }
    }
    }

    The only way you can abstract even further is with a closure that accepts the the action behaviour as an arguement. That would give something more like this:

    class DealerController {
    private validateAction = { action ->
    Dealer dealerInstance = Dealer.get(params.id)
    if (!validateInstanceExists(dealerInstance) || !validateAccountInExternalService(dealerInstance) ||
    ifInstanceWasConcurrentlyModified(dealerInstance, params.version)) {
    redirectToList()
    } else {
    action(dealerInstance)
    }
    }

    @Transactional
    def update() {
    validateAction { dealerInstance ->
    dealerInstance.properties = params
    saveUpdatedInstance(dealerInstance)
    redirectToAfterUpdate(dealerInstance)
    }
    }
    }

    ReplyDelete