There are different paradigms of programming (for example, functional, imperative, and object-oriented) and styles (for example, compact, verbose, minimalistic, and too-clever). Therefore, the candidates for refactoring are different from one person to the other.

There is another way, as opposed to subjectively, of finding candidates for refactoring: objectively. Research papers investigating ways of finding these ways include:

Source: http://www.bth.se/fou/cuppsats.nsf/all/2e48c5bc1c234d0ec1257c77003ac842/$file/BTH2014SIVERLAND.pdf

After getting to know the code more, it seems that the most important functional change is to replace the current status (fragment):

@XmlRootElement
public class Book {

    private final String title;
    private final String author;
    private int status; //<- this attribute
    private int id;

with a collection of them (fragment):

@XmlRootElement
public class Book {
    private int[] statuses;
    // ...

This might seem to work (after changing all access to the field to the array, for example), but this also prompts a functional requirement.

The Alexandria software should be able to store books and lend them to users who have the power to return them. The user can also query the system for books, by author, book title, status, and ID.

The Product Owner (PO) confirms that searching books via status has now changed: now it also allows to search for any previous status.

This change is getting bigger and bigger. Whenever we feel that the time for removing this legacy code has come, we start applying the legacy code algorithm.

We have also detected a primitive obsession and feature envy smell; storing the status as int (primitive obsession) and then actuating on another object's state (feature envy). We will add this to the following to-do list:

The first step we've decided to take to ensure the behavior is maintained during the a refactoring is to write end-to-end tests. In other applications that include frontend, this could be using a higher-level tool such as Selenium/Selenide.

In our case, as the frontend is not subject to refactoring, the tool can be lower-level. We have chosen to write HTTP requests for the purpose of end-to-end tests.

These requests should be automatic and testable, and should follow all existing rules for automatic tests or specifications. As we were discovering the real application behavior while writing these tests, we have decided to write a spike in a tool called Postman. (The tool can be found here: https://chrome.google.com/webstore/detail/postman-rest-client/fdmmgilgnpjigdojojpjoooidkmcomcm. The product website is here: https://www.getpostman.com/). This is also possible with a tool called curl (http://curl.haxx.se/).

To do this, we decide to execute the legacy software locally with the following line:

mvn clean jetty:run

This fires up a local jetty server that processes requests. The big benefit is that deployment is done automatically and there is no need to package everything and manually deploy to an application server (for example, JBoss AS, GlassFish, Geronimo, and TomEE). This can greatly speed up the process of making changes and seeing the effects, therefore decreasing the feedback lead time. Later on, we will start the server programmatically from Java code.

We start looking for functionalities. As we discovered earlier that the BooksEndpoint class contains the webservice endpoint definitions, this is a good place to start looking for functionalities. They are listed below:

We launch the server manually and start writing requests:

These tests seem good enough for a spike. One thing that we have realized is that each response contains a timestamp, so this makes our automation more difficult:

For the tests to have more value, they should be automated and exhaustive. For the moment, they are not, so we consider them spikes. They will be automated in the future.

After writing down all the requests, these are the states that we have found in the application, represented by a state diagram:

After these tests are ready and we start to understand the application, it is time to automate the tests. After all, if they are not automated, we don't really feel confident enough for refactoring.

We proceed to start the server programmatically. For this, we have decided to use Grizzly (https://grizzly.java.net/), which allows us to start the server using the configuration from Jersey's ResourceConfig (FQCN: org.glassfish.jersey.server.ResourceConfig), as shown on the test BooksEndpointTest (fragment):

The code can be found at https://bitbucket.org/vfarcic/tdd-chapter-08/commits/branch/refactor/inject-dependencies:

public class BooksEndpointTest {
    public static final URI FULL_PATH = 
      URI.create("http://localhost:8080/alexandria");
    private HttpServer server;

    @Before
    public void setUp() throws IOException {
        ResourceConfig resourceConfig =
          new MyApplication();
        server = GrizzlyHttpServerFactory
          .createHttpServer(FULL_PATH, resourceConfig);
        server.start();
    }

    @After
    public void tearDown(){
        server.shutdownNow();
    }

This prepares a local server in the address http://localhost:8080/alexandria. It will only be available for a short period of time (while the tests run), so if you need to manually access the server, whenever you want to pause the execution, insert a call to the following method:

public void pauseTheServer() throws Exception {
    System.in.read();
}

When you want to stop the server, stop the execution or hit Enter in the allocated console.

Now we can start the server programmatically, pause it (with the preceding method), and execute the spike again. The results are the same, therefore the refactor is successful.

We add the first automated test to the system:

The code can be found at https://bitbucket.org/vfarcic/tdd-chapter-08/commits/branch/refactor/inject-dependencies:

public class BooksEndpointTest {

    public static final String AUTHOR_BOOK_1 = 
      "Viktor Farcic and Alex Garcia";
    public static final String TITLE_BOOK_1 =
      "TDD in Java";
    private final Map<String, String> TDD_IN_JAVA;

    public BooksEndpointTest() {
        TDD_IN_JAVA = getBookProperties(TITLE_BOOK_1, AUTHOR_BOOK_1);
    }

    private Map<String, String> getBookProperties
      (String title, String author) {
        Map<String, String> bookProperties =
          new HashMap<>();
        bookProperties.put("title", title);
        bookProperties.put("author", author);
        return bookProperties;
    }

    @Test
    public void add_one_book() throws IOException {
        final Response books1 = addBook(TDD_IN_JAVA);
        assertBooksSize(books1, is("1"));
    }

     private void assertBooksSize(Response response, Matcher<String> matcher) {
        response.then().body(matcher);
    }

    private Response addBook
      (Map<String, ?> bookProperties) {
        return RestAssured
            .given().log().path()
            .contentType(ContentType.URLENC)
            .parameters(bookProperties)
            .post("books");
    }

For testing purposes, we're using a library called RestAssured (https://code.google.com/p/rest-assured/) that allows for easier testing of REST and JSON.

To complete the automated test suite, we create these tests:

The code is found in https://bitbucket.org/vfarcic/tdd-chapter-08/commits/branch/refactor/inject-dependencies:

Now that we have a suite that ensures that no regression is introduced, we take a look at the following to-do list:

We will tackle the dependency injection first.

The creation of the BookRepository dependency is in BooksEndpoint (fragment):

@Path("books")
@Component
public class BooksEndpoint {

    private BooksRepository books =
      new BooksRepository();
[…]

We can unify the launchers accepting a BooksEndpoint dependency: if we don't specify it, it will register the dependency with the real instance of BooksRepository. Otherwise, it will register the received one:

public class MyApplication
      extends ResourceConfig {

    public MyApplication() {
        this(new BooksEndpoint(
          new BooksRepository()));
    }

    public MyApplication
      (BooksEndpoint booksEndpoint) {
        register(booksEndpoint);
        register(RequestContextFilter.class);
        register(JacksonJaxbJsonProvider.class);
        register(CustomExceptionMapper.class);
    }
}

In this case, we have opted for 'constructor chaining' to avoid repetition in the constructors.

After doing this refactor, the BooksEndpointInteractionTest class is as follows, in its final state:

public class BooksEndpointInteractionTest {

    public static final URI FULL_PATH = URI.
        create("http://localhost:8080/alexandria");
    private HttpServer server;
    private BooksRepository booksRepository;

    @Before
    public void setUp() throws IOException {
        booksRepository = mock(BooksRepository.class);
        BooksEndpoint booksEndpoint =
          new BooksEndpoint(booksRepository);
        ResourceConfig resourceConfig =
          new MyApplication(booksEndpoint);
        server = GrizzlyHttpServerFactory
           .createHttpServer(FULL_PATH, resourceConfig);
        server.start();
    }

The first test passed, so we can mark the dependency injection task as done.

The to-do list:

Once we have the necessary test environment in place, we can add the new feature.

As a library manager, I want to know all the history for a given book so that I can measure which books are more in demand than others.

We will start with a red specification:

public class BooksSpec {

    @Test
    public void should_search_for_any_past_state() {
        Book book1 = new Book("title", "author", States.AVAILABLE);
        book1.censor();

        Books books = new Books();
        books.add(book1);

        String available =
          String.valueOf(States.AVAILABLE);
        assertThat(
          books.filterByState(available).isEmpty(), is(false));
    }
}

Run all the tests and see the last one fail.

Implement the search on all states (fragment):

public class Book {

    private ArrayList<Integer> status;

    public Book(String title, String author, int status) {
        this.title = title;
        this.author = author;
        this.status = new ArrayList<>();
        this.status.add(status);
    }

    public int getStatus() {
        return status.get(status.size()-1);
    }

     public void rent() {
        status.add(States.RENTED);
    }
    [...]

    public List<Integer> anyState() {
        return status;
    }
    [...]

In this fragment, we have omitted the irrelevant parts: things that were not modified or more modifier methods such as rent that have changed the implementation in the same fashion:

public class Books {
    public Books filterByState(String state) {
        Integer expectedState = Integer.valueOf(state);
        return new Books(
            new ConcurrentLinkedQueue<>(
                books.stream()
                  .filter(x -> x.anyState()
                  .contains(expectedState))
                  .collect(toList())));
    }
    [...]

The outside methods, especially the serialization to JSON, are not affected as the method getStatus still returns an int value.

We run all the tests and everything is green.

The to-do list: