Refactoring with TDD

As everybody knows, good software engineering recommends to respect separation of concerns. For example, controller’s main responsibilities are to process incoming HTTP requests, to invoke some business logic and to dispatch the response.

You can see that the following controller doesn’t respect this recommendation since the searching logic is implemented directly into the controller method.

package todo
 
class TaskController {
 
    static scaffold = true
 
    def list() {
        params.max = Math.min(params.max ? params.int('max') : 5, 100)
 
        def taskList = Task.createCriteria().list (params) {
            if ( params.query ) {
                ilike("description", "%${params.query}%")
            }
        }
 
        [taskInstanceList: taskList, taskInstanceTotal: taskList.totalCount]
    }
 
}

Obviously, it’s a very simple example and most people would say that it’s not a big deal but let me show you how to refactor this code to respect separation of concerns.

The most common places to implement business logic in Grails are services and domain classes. In our case, a service would be unnecessary at this point so let’s move business logic to the Task domain class.

This time, I’ll use Test Driven Development so I’m writing the test first:

// file: test/unit/todo/TaskSpec.groovy
package todo

import grails.test.mixin.TestFor

import spock.lang.Specification
import spock.lang.Unroll

@TestFor(Task)
class TaskSpec extends Specification {

	@Unroll
	def "search() should return #count results for query '#query'"() {
	    given:
            [
                new Task(description: "Buy bread"),
                new Task(description: "Buy milk"),
	        ]*.save()
	    expect:
            count == Task.search(query).count()
	    where:
    	    count   | query
    	    2       | null
    	    2       | "Buy"
    	    1       | "Bread"
    	    0       | "iPod"
	}

}

This is a Spock unit test:

  • it first creates and saves 2 tasks.
  • it then passes a query string to the Task domain class
  • it then asserts the number of matching results for various queries

If you run this test with the command grails test-app unit:spock TaskSpec, you should see the following:

| Running 1 spock test... 1 of 1
| Failure:  search() should return '2' results for query 'null'(todo.TaskSpec)
|  groovy.lang.MissingMethodException: No signature of method: todo.Task.search() is applicable for argument types: (null) values: [null]
[some output omitted]
| Completed 1 spock test, 4 failed in 362ms
| Tests FAILED  - view reports in target/test-reports

All 4 test cases fail as Grails complains that it can’t find any search method in the Task class. Let’s create one with a named query:

package todo

class Task {

    String description

    static namedQueries = {
        search { query ->
        }
    }

}

Here is what Grails outputs if we run the same command again:

| Running 1 spock test... 3 of 1
| Failure:  search() should return '1' results for query 'Bread'(todo.TaskSpec)
|  Condition not satisfied:

count == Task.search(query).count()
|     |       |      |      |
1     false   |      Bread  2
              org.grails.datastore.gorm.query.NamedCriteriaProxy@5d5ef3e7

	at todo.TaskSpec.search() should return '#count' results for query '#query'(TaskSpec.groovy:19)
| Running 1 spock test... 4 of 1
| Failure:  search() should return '0' results for query 'iPod'(todo.TaskSpec)
|  Condition not satisfied:

count == Task.search(query).count()
|     |       |      |      |
0     false   |      iPod   2
              org.grails.datastore.gorm.query.NamedCriteriaProxy@74ef95c6

	at todo.TaskSpec.search() should return '#count' results for query '#query'(TaskSpec.groovy:19)
| Completed 1 spock test, 2 failed in 762ms
| Tests FAILED  - view reports in target/test-reports

OK, no more groovy.lang.MissingMethodException but some test cases still fail. So let’s implement the search named query. All we have to do is to move the business logic from the controller method to the domain class:

package todo

class Task {

    String description

    static namedQueries = {
        search { query ->
            if ( query ) {
                ilike("description", "%${query}%")
            }
        }
    }

}

And now, all test cases pass:

| Completed 1 spock test, 0 failed in 329ms
| Tests PASSED - view reports in target/test-reports

Since we moved the search logic to the domain class, we can now remove it from the controller:

package todo
 
class TaskController {
 
    static scaffold = true
 
    def list() {
        params.max = Math.min(params.max ? params.int('max') : 5, 100)
 
        def taskList = Task.search(params.query).list(params)
 
        [taskInstanceList: taskList, taskInstanceTotal: taskList.totalCount]
    }
 
}

We successfully refactored our controller method to move business logic where it belongs.

Advertisements

One thought on “Refactoring with TDD

  1. You should really test the controller as well. In this example it’s clear that that it’s correct, but if you were refactoring a more complex controller action in a series of refactorings I’d want that safety net!

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s