Part 3: Recursive menu building

Part 3 of the Freemarker cleanup involved refactoring menu creation. The application has several menus that appear on the left side of the browser window. These menus are either nested inside eachother or stacked on top of one another.

Before part 3, the creation of the menus was done by one template called main.ftl. This template included other templates (and so on). This was main.ftl:

[#ftl]

[#if user.member]
	[#include "/member-menu.ftl"]
[#else]
	[#include "/cross-role-menu.ftl"]
[/#if]
[#if someCondition?? ]
	[#include "/some-menu.ftl"]
[/#if ]
	[#if foo.id !=  user.id]
		[#include "/foo-menu.ftl"]
[/#if ]

All of the menu templates had some conditional logical statements in them – in fact, there was probably an average of about a dozen per template. The only place that these logic statements were tested was in the Selenium tests – obviously not ideal.

The aim of this refactoring was to get rid of all the conditional logic in the templates. The logic would be replaced with tested Java code. This made it possible to render the menus from one single recursive menu template:

[#ftl]
[#import "/spring.ftl" as spring /]

[#if menu??]
    [@buildMenu menu=menu depth=0/]
[/#if]

[#macro buildMenu menu depth]
    [#list menu.menuEntries as menuEntry]
        [#if menuEntry.type == "LINK"]
            <div>
                <a href="[@spring.url menuEntry.link?html /]">${menuEntry.caption?html}</a>
            </div>
        [#else]
            [#if menuEntry.menuEntries?size > 0]
                [#if depth > 0]
                    <h3>${menuEntry.caption}</h3>
                    <div id="${menuEntry.name}" class="menu_sub_block">
                [#else]
                    <div id="${menuEntry.name}" class="menu_block">
                    <h5>${menuEntry.caption}</h5>
                [/#if]
                [@buildMenu menu=menuEntry depth=depth+1/]
                </div>
            [/#if]
        [/#if]
    [/#list]
[/#macro]

The new template builds all the menus from the menu model entry. This is added into the model in the MenuInterceptor. This interceptor has dependencies on four factories that create the required menus. The postHandle method creates a rootMenu that contains the other menus:

public class MenuInterceptor extends HandlerInterceptorAdapter {
    private FooMenuFactory fooMenuFactory;
    private CrossRoleMenuFactory crossRoleMenuFactory;
    private MemberMenuFactory memberMenuFactory;
    private SomeMenuFactory someMenuFactory;

    @Autowired
    public void setFooMenuFactory(FooMenuFactory fooMenuFactory) {
        this.fooMenuFactory = fooMenuFactory;
    }

    // omitted setters for the other factories

    @Override
    public void postHandle(HttpServletRequest request, HttpServletResponse response, Object handler, ModelAndView mv)
            throws Exception {

        if (mv != null) {
            User user = (User) request.getAttribute(RequestAttributeNames.user.name());

            Menu rootMenu = new Menu("menu");
            Menu memberMenu = memberMenuFactory.createMemberMenu(user);
            Menu crossRoleMenu = crossRoleMenuFactory.createCrossRoleMenu(user);
            Menu someMenu = someMenuFactory.createSomeMenu(user);
            Menu fooMenu = fooMenuFactory.createFooMenu(user);
            rootMenu.addEntries(memberMenu, crossRoleMenu, someMenu, fooMenu);

            mv.addObject("menu", rootMenu);
        }
    }
}

Each of the menu factories creates a Menu object. The Menu class has a list of MenuEntry objects. There are two implementations of the MenuEntry interface: Link and Menu:

The MenuEntry interface:

public interface MenuEntry {

    String getCaption();

    MenuEntryType getType();
}

Menu:

public class Menu implements MenuEntry {
    private String caption = "";

    private List<MenuEntry> menuEntries;
    private final String name;

    public Menu(String name) {
        this.name = name;
        menuEntries = Lists.create();
    }

    public void setCaption(String caption) {
        this.caption = caption;
    }

    public void addEntry(MenuEntry menuItem) {
        menuEntries.add(menuItem);
    }

    public String getCaption() {
        return caption;
    }

    public List<MenuEntry> getMenuEntries() {
        return menuEntries;
    }

    public void addEntries(MenuEntry... links) {
        menuEntries.addAll(Arrays.asList(links));
    }

    public MenuEntryType getType() {
        return MenuEntryType.MENU;
    }

    public String getName() {
        return name;
    }
}

… and Link:

public class Link implements MenuEntry {

    private final String captionText;
    private final SecureLinks.Link link;

    public SecureContextLink(String captionText, SecureContextLinks.Link link) {
        this.captionText = captionText;
        this.link = link;
    }

    public String getCaption() {
        return captionText;
    }

    public String getLink() {
        return link.toString();
    }

    public MenuEntryType getType() {
        return MenuEntryType.LINK;
    }
}

Each of the factories creates a menu using the logic that was previously in the Freemarker templates. Each factory was developed using TDD and looks a bit like this:

public class FooMenuFactory {
    private FooRepository fooRepository;
    private final SecureContextLinks links = new SecureContextLinks();

    public FooMenuFactory(FooRepository fooRepository) {
        this.fooRepository = fooRepository;
    }

    public Menu createFooMenu(User user) {
        Menu fooMenu = new Menu("foo_menu");

        fooMenu.setCaption(user.getDisplayName());

        if (context.isMember()) {
           fooMenu.addEntry(new SecureContextLink("Summary", links.getSomeSummary()));
        }

        // add other links

        return fooMenu;
    }
}

Moving to this style of menu creation reduced 7 templates down to one recursive template. All of the logic that was previously in the templates was moved into Java and was unit tested. (Yay!)

Part 2: SecureLinks

Part two of the Freemarker cleanup came about when we noticed that we were adding the same links many times into various models. This repetition was eliminated by adding the most common links into the model using an interceptor.

The interceptor has a postHandle method that adds an instance of our new SecureLinks class (below) into the model:

@Override
public void postHandle(HttpServletRequest request, HttpServletResponse response, Object handler, ModelAndView mv)
        throws Exception {

    if (mv != null) {
        mv.addObject("links", new SecureLinks());
    }
}

The SecureLinks class provides the most commonly used links in our application:

import my.app.LinkBuilder;
import my.app.QueryString;
import my.app.controller.secure.foo.FooController;
import my.app.controller.secure.bar.BarController;

public class SecureLinks {

    private static final LinkBuilder BUILDER = new SecureLinkBuilder();

    public Link getFooSearch() {
        return new PlainLink(FooController.class);
    }

    public Link getBarList() {
        return new LinkWithQueryString(BarController.class, QueryString().add("letter", "A"));
    }

    // other methods omitted for brevity

    public static interface Link {
    }

    public static class PlainLink implements Link {
        private final Class<?> controller;
        private final String methodName;

        public PlainLink(Class<?> controller) {
            this(controller, null);
        }

        public PlainLink(Class<?> controller, String methodName) {
            this.controller = controller;
            this.methodName = methodName;
        }

        @Override
        public String toString() {
            if (methodName != null) {
                return BUILDER.linkTo(controller, methodName);
            }
            return BUILDER.linkToGet(controller);
        }
    }

    public static class LinkWithQueryString implements Link {
        private final Class<?> controller;
        private final String methodName;
        private final QueryString query;

        public LinkWithQueryString(Class<?> controller, String methodName, QueryString query) {
            this.controller = controller;
            this.methodName = methodName;
            this.query = query;
        }

        public LinkWithQueryString(Class<?> controller, QueryString query) {
            this(controller, null, query);
        }

        @Override
        public String toString() {
            if (methodName != null) {
                return BUILDER.linkTo(controller, methodName, query);
            }
            return BUILDER.linkToGet(controller, query);
        }
    }
}

Adding the links into the model in the interceptor means that there is something extra in each ModelMap that may not necessarily be used/required, but we thought that it was worth it to remove the repetition.

Java LinkBuilder

Shortly before Christmas Tom and I decided to try to remove logic from some project Freemarker templates. Our first step was to move away from building up URIs inside templates, by creating some sort of Java URI builder.

Background:
We had lots of Freemarker template snippets that looked like this:

[#if member.someType.value != 'Foo']
     <div>[@macros.link_to 'Bar reports' 'member/report?id=${id}&amp;type=M&amp;database=${database}' /]</div>
[/#if]

All of them used the link_to Freemarker macro, which was defined as:

[#macro link_to caption path target='_top']
    [#if path?starts_with("/")]
        [#assign newPath=path?substring(1) /]
    [#else]
        [#assign newPath=path /]
    [/#if]
    <a href="[@spring.url '/appSecureServletPath/${newPath}' /]" target="${target}">${caption}</a>
[/#macro]

What we wanted to achieve first up:

Our aim was to replace all the uses of the link_to macro and instead generate all the URIs in Java and then add them to the model. We wanted to move a template usage like this:

<a href="[@spring.url contactDetailsChange?html /]">Change your contact details</a>

Where the link is added in the controller like so:

modelAndView.addObject("contactDetailsChange", linkBuilder.linkTo(ContactDetailsController.class));

Creating the LinkBuilder:

We did an assessment of all the URI endpoints in our project and realised that:
- We have some controller classes with a request mapping, some controllers that have methods with request mappings and some controllers with a combination of both class and method mappings.
- We have some controllers with more than one GET request method mapping and therefore could not assume only one request method mapping per controller.

We test drove a LinkBuilder interface and a DefaultLinkBuilder implementation for all the combinations we found in the controllers. The idea was to link from one handler class/method to another without being concerned about the specific URI mapped paths.

We also included methods that return a ModelAndView for redirecting and forward from a controller. Some of the methods also take a QueryString microtype (see below).

This is the LinkBuilder interface:

import org.springframework.web.bind.annotation.RequestMethod;
import org.springframework.web.servlet.ModelAndView;

public interface LinkBuilder {

    String linkTo(Class<?> controller, String methodName);

    String linkTo(Class<?> controller, RequestMethod method);

    String linkTo(Class<?> controller, String methodName, QueryString query);

    String linkTo(Class<?> controller, RequestMethod method, QueryString query);

    String linkToGet(Class<?> controller);

    String linkToGet(Class<?> controller, QueryString query);

    String forwardTo(Class<?> controller, String methodName);

    ModelAndView redirectTo(Class<?> controller, String method);

    ModelAndView redirectTo(Class<?> controller);

    ModelAndView redirectTo(Class<?> controller, String method, QueryString query);

    ModelAndView redirectTo(Class<?> controller, QueryString query);
}

And the DefaultLinkBuilder:

import org.apache.commons.lang.ArrayUtils;
import org.springframework.core.annotation.AnnotationUtils;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestMethod;
import org.springframework.web.servlet.ModelAndView;

import java.lang.reflect.Method;

public class DefaultLinkBuilder implements LinkBuilder {

    private final String prefix;

    public DefaultLinkBuilder() {
        this("");
    }

    public DefaultLinkBuilder(String prefix) {
        this.prefix = prefix;
    }

    public String linkTo(Class<?> controller, String methodName) {
        return prefix + getControllerUrl(controller) + getMethodUrl(controller, methodName);
    }

    public String linkTo(Class<?> controller, RequestMethod method) {
        return prefix + getControllerUrl(controller) + getMethodUrl(controller, method);
    }

    public String linkTo(Class<?> controller, String methodName, QueryString query) {
        return linkTo(controller, methodName) + "?" + query;
    }

    public String linkTo(Class<?> controller, RequestMethod method, QueryString query) {
        return linkTo(controller, method) + "?" + query;
    }

    public String linkToGet(Class<?> controller) {
        return linkTo(controller, RequestMethod.GET);
    }

    public String linkToGet(Class<?> controller, QueryString query) {
        return linkTo(controller, RequestMethod.GET, query);
    }

    public String forwardTo(Class<?> controller, String methodName) {
        return "forward:" + linkTo(controller, methodName);
    }

    public ModelAndView redirectTo(Class<?> controller, String method) {
        return new ModelAndView("redirect:" + linkTo(controller, method));
    }

    public ModelAndView redirectTo(Class<?> controller) {
        return new ModelAndView("redirect:" + linkTo(controller, RequestMethod.GET));
    }

    public ModelAndView redirectTo(Class<?> controller, String method, QueryString query) {
        ModelAndView mv = redirectTo(controller, method);
        query.addToModel(mv.getModel());
        return mv;
    }

    public ModelAndView redirectTo(Class<?> controller, Context context) {
        QueryString queryString = context.asQueryString();
        return redirectTo(controller, queryString);
    }

    public ModelAndView redirectTo(Class<?> controller, QueryString query) {
        ModelAndView mv = redirectTo(controller);
        query.addToModel(mv.getModel());
        return mv;
    }

    private String getControllerUrl(Class<?> controller) {
        RequestMapping annotation = AnnotationUtils.findAnnotation(controller, RequestMapping.class);
        if (annotation != null) {
            return getFirstValue(annotation);
        }
        return "";
    }

    private String getMethodUrl(Class<?> controller, String methodName) {
        Method[] methods = controller.getMethods();
        for (Method method : methods) {
            if (method.getName().equals(methodName)) {
                RequestMapping annotation = AnnotationUtils.findAnnotation(method, RequestMapping.class);
                if (annotation != null) {
                    return getFirstValue(annotation);
                }
            }
        }
        throw new IllegalArgumentException("Cannot find method with name " + methodName
                + " with a RequestMapping annotation on controller " + controller.getName());
    }

    private String getMethodUrl(Class<?> controller, RequestMethod requestMethod) {
        Method[] methods = controller.getMethods();
        for (Method method : methods) {
            RequestMapping annotation = AnnotationUtils.findAnnotation(method, RequestMapping.class);
            if (annotation != null) {
                if (ArrayUtils.contains(annotation.method(), requestMethod)) {
                    return getFirstValue(annotation);
                }
            }
        }
        throw new IllegalArgumentException("Cannot find method that can handle " + requestMethod
                + " requests on controller " + controller.getName());
    }

    private String getFirstValue(RequestMapping annotation) {
        String[] value = annotation.value();
        if (value.length > 0) {
            return value[0];
        }
        return "";
    }
}

We have two subclasses of the DefaultLinkBuilder the SecureLinkBuilder and the UnsecureLinkBuilder. These classes simply set the appropriate servlet path as the prefix.

The QueryString class knows how to add itself into the model:

import org.apache.commons.httpclient.NameValuePair;
import org.apache.commons.httpclient.util.EncodingUtil;

import java.util.List;
import java.util.Map;

public class QueryString {

    private final List<NameValuePair> pairs = Lists.create();

    public QueryString add(String name, String value) {
        pairs.add(new NameValuePair(name, value));
        return this;
    }

    public boolean isEmpty() {
        return pairs.isEmpty();
    }

    public NameValuePair[] toArray() {
        return pairs.toArray(new NameValuePair[pairs.size()]);
    }

    public void addToModel(Map<String, Object> model) {
        for (NameValuePair pair : pairs) {
            model.put(pair.getName(), pair.getValue());
        }
    }

    @Override
    public String toString() {
        return EncodingUtil.formUrlEncode(toArray(), "UTF-8");
    }
}

The creation of the LinkBuilder and the implementations allowed us to go through all the controllers and add links into the models. We removed a lot of string concatenation in templates doing this.

Book Club: Working Effectively With Legacy Code – Chapters 17, 18 and 19 (Michael Feathers)

This week at book club we continued the three-chapter theme, taking on chapters 17, 18 and 19 of Working Effectively With Legacy Code. Chapter 17 is the most lengthy of the three and we spent almost all of our time discussing that one.

Chapter 17: My Application Has No Structure

In this chapter Feathers discusses why code bases degrade. He says that “They (applications) might have started out with a well-thought-out architecture, but over the years, under schedule pressure, they can get to the point where nobody really understands their complete structure.” He gives three possible reasons why a team may be unaware of this happening:

• The system is so complex that it takes a long time to get the big picture
• The system can be so complex that there is no big picture
• The team is in a very reactive mode, dealing with emergency after emergency so much that they lose sight of the big picture

Feathers details some suggestions for how to communicate the intent of a system including ‘Telling the Story of the System’, a process where “One person starts off by asking the other ‘What is the architecture of the system?’ Then the other person tries to explain the architecture of the system using only a few concepts.” This is useful because “Often when we force ourselves to communicate a very simple view of the system, we can find new abstractions.”

• I think all of us have used this technique when first coming to a project to try and get an understanding of the system. I usually have this type of conversation with each pair for the first couple of weeks on a project.
• Ahrum said that in his experience developers find it very difficult to explain a system simply because we generally go into too much detail. Giving information about logging and security are examples including too much noise in the explanation.
• Tom said that it’s important to speak with everyone on the project, not just the architect. He finds that architects generally focus too much on application layering.
• I think this technique is related to Dan’s idea of having a project shaman.

Feathers also discussed Naked CRC. This is a lightweight version of CRC where there is no writing on the cards. “The person describing he system uses a set of blank index cards and lays them down on a table one by one. He or she can move the cards, point at them, or do whatever else is needed to convey the typical objects in the system and how they interact”. I’ve never used this approach before but Anita and I both said that we would like to try it because we think we’re visual learners. Tom suggested that combining the cards and a whiteboard could also be useful, especially to show interactions between components.

We also spent some time discussing other techniques for getting the big picture of an application, especially when encountering it for the first time:

• Reading the code can be a good way to spot major concepts or the lack of them.
• Raphael said that it’s useful to listen to the language that people use to describe the system and to compare that to the concepts in the code. Sarah and Mark both recently posted on this.
• Tom said that he likes to make lots of mindmaps when he first goes onto a new project. He said he generally likes to observe for about a month before he attempts any large refactorings.

Adding a Spring aspect

Yesterday Tom and I added a Spring aspect to our front end project. My only previous encounter with aspects was when I read a Spring book about a year ago. So, what is an aspect? According to the documentation “Aspects enable the modularization of concerns such as transaction management that cut across multiple types and objects”.

Tom and I needed to redirect the user to an ‘eek’ page if they tried to do some transactions when their account was in a bad way. An aspect seemed like a tool for the job.  But what sort of aspect? As Tom explained to me, the Around advice is the only type that can be used to change the flow of execution:

Around advice: Advice that surrounds a join point such as a method invocation…. Around advice can perform custom behavior before and after the method invocation. It is also responsible for choosing whether to proceed to the join point or to shortcut the advised method execution by returning its own return value or throwing an exception.

We created the new aspect in these steps:

1. Create a new method annotation. This annotation was applied to all the controller method join points. According to the Spring documentation this is what is known as an “@annotation pointcut designator – the subject of the join point matches only methods that have the given annotation.”

@Target({ElementType.METHOD})
@Retention(RetentionPolicy.RUNTIME)
public @interface TransactionPermitted {
}

2. Create the aspect using TDD. Our aspect checks to see whether the the users account is in a state and redirects to the “transactions-not-permitted” view if this is the case. We used the example in the documentation to create our aspect. Tom pointed out a couple of thing while we were doing this that I think are important to remember:

• Aspects need a no argument constructor.
• From the docs: “The value returned by the around advice will be the return value seen by the caller of the method”. We had to make sure that all the handler methods advised by this aspect also return a string (and not a ModelAndView for example).

@Aspect
@Component
public class TransactionPermittedAspect {

private final Logger logger = Logger.getLogger(TransactionPermittedAspect.class);

private FooRepository fooRepository;

@Autowired
public void setFooRepository(FooRepository fooRepository) {
this.fooRepository = fooRepository;
}

@Around("@annotation(my.package.TransactionPermitted)")
public Object intercept(ProceedingJoinPoint pjp) throws Throwable {
logger.debug("Checking whether transactions are permitted");

Widget widget = AspectUtils.getArgumentOfType(pjp.getArgs(), Widget.class);
Bar bar = fooRepository.getBar(widget);

if (bar.hasASomeBadCondition()) {
return "transactions-not-permitted";
}
return pjp.proceed();
}

}

Our aspect (above) uses AspectUtils. This class, created by Tom, abstracts the argument searching and casting:

public class AspectUtils {

public static <T> T getArgumentOfType(Object[] args, Class<T> type) {
for (Object arg : args) {
if (type.isInstance(arg)) {
return type.cast(arg);
}
}
throw new IllegalArgumentException("Cannot find argument of " + type);
}
}

3. Apply the annotation pointcut to the target handler methods. This step was trivial, but adding them using TDD was interesting. Each class with a method annotated by our pointcut now has a test like this:

@Test
public void shouldCheckThatTransactionsArePermittedForFoo() {
assertNotNull("Transaction permission check not found", myMethod().getAnnotation(TransactionPermitted.class));
}

private Method myMethod() {
return MethodFinder.findRequestHandlerMethod(WibbleController.class, "myMethod");
}

Note that the MethodFinder looks for the target method with the a particular name and a RequestMapping annotation:

import static org.junit.Assert.fail;
import org.springframework.web.bind.annotation.RequestMapping;

import java.lang.reflect.Method;

public class MethodFinder {

public static Method findRequestHandlerMethod(Class<?> type, String methodName) {
Method[] methods = type.getMethods();
for (Method method : methods) {
if (method.getName().equals(methodName) && (method.getAnnotation(RequestMapping.class) != null)) {
return method;
}
}
fail("Cannot find method named [" + methodName + "] with RequestMapping annotation in " + type.getName());
return null;
}
}

}

Adding this around aspect took us a very short time and it perfectly suited our need.

DefaultHttpClientIntegrationTest

Today Tom and I were looking at our DefaultHttpClient class. This class wraps/adapts an Apache HttpClient. It implements our HttpWebClient interface:

public interface HttpWebClient {

    public HttpResponse post(String endpointURL, String requestBody);

    public HttpResponse get(String endpointURL, QueryString query);
}

Until today we didn’t have any tests for just this class, although there was some coverage from integration tests for other classes. We added tests that are very similar to some written by Cam and Silvio in another project (full credit to them). There’s a couple of interesting things happening our test class (below):

• A Jetty server is created in the setup method. The DefaultHttpWebClient test instance makes a request to this server.

• The findFreePort method uses java.net.ServerSocket to identify a free port for a Jetty Server to run on.

• Each test sets the handler for the test Jetty server as a new TestHandler (line 84). The TestHandler class implements the Jetty AbstractHandler interface. It delegates to an overloaded handle method that is defined in each test method.

• In the shouldPerformHttpPost test the handle method returns a response with an OK status code. It also puts some information into a map called ‘inspector’. Request information is put into the ‘inspector’ and it is used in several assertions.

Here tis:

public class DefaultHttpWebClientIntegrationTest {

    private int freePort;
    private Server server;

    @Before
    public void setup() throws Exception {
        freePort = findFreePort();
        server = new Server(freePort);
        server.start();
    }

    @After
    public void teardown() throws Exception {
        server.stop();
    }

    private int findFreePort() throws Exception {
        ServerSocket socket = new ServerSocket(0);
        int port = socket.getLocalPort();
        socket.close();
        return port;
    }

    @Test
    public void shouldPerformHttpPost() {
        final Map<String, String> inspector = Maps.create();

        server.setHandler(new TestHandler() {
            public void handle(HttpServletRequest request, HttpServletResponse response) throws IOException {

                inspector.put("method", request.getMethod());
                inspector.put("contentType", request.getContentType());
                inspector.put("body", IOUtils.toString(request.getInputStream()));

                response.setStatus(HttpServletResponse.SC_OK);
                response.setContentType("text/plain");
                response.getWriter().print("test response");
            }
        });

        DefaultHttpWebClient client = new DefaultHttpWebClient();
        HttpResponse response = client.post("http://localhost:" + freePort + "/test", "test payload");

        assertThat(inspector.get("method"), equalTo("POST"));
        assertThat(inspector.get("contentType"), equalTo("text/xml; charset=UTF-8"));
        assertThat(inspector.get("body"), equalTo("test payload"));

        assertThat(response.isOK(), equalTo(true));
        assertThat(response.getContentType(), startsWith("text/plain"));
        assertThat(response.getResponseBodyAsText(), equalTo("test response"));
    }

    @Test
    public void shouldPerformHttpGet() {
        final Map<String, String> inspector = Maps.create();

        server.setHandler(new TestHandler() {
            public void handle(HttpServletRequest request, HttpServletResponse response) throws IOException {

                inspector.put("method", request.getMethod());
                inspector.put("foo", request.getParameter("foo"));

                response.setStatus(HttpServletResponse.SC_OK);
                response.setContentType("text/plain");
                response.getWriter().print("test response");
            }
        });

        QueryString queryString = new QueryString();
        queryString.add("foo", "bar");

        DefaultHttpWebClient client = new DefaultHttpWebClient();
        HttpResponse response = client.get("http://localhost:" + freePort + "/test", queryString);

        assertThat(inspector.get("method"), equalTo("GET"));
        assertThat(inspector.get("foo"), equalTo("bar"));

        assertThat(response.isOK(), equalTo(true));
        assertThat(response.getContentType(), startsWith("text/plain"));
        assertThat(response.getResponseBodyAsText(), equalTo("test response"));
    }

    private static abstract class TestHandler extends AbstractHandler {

        public void handle(String target, HttpServletRequest request, HttpServletResponse response, int dispatch)
                throws IOException, ServletException {

            handle(request, response);
            ((Request) request).setHandled(true);
        }

        public abstract void handle(HttpServletRequest request, HttpServletResponse response) throws IOException;
    }
}

Mock Objects

Over the course of the last year or so I’ve been learning to play tennis. I’ve definitely improved from where I started (although it’s not hard to improve from disastrously bad). Last night I was playing and noticed that my serve had deteriorated somewhat. The solution to getting it on track was to go back to basics and practice doing the simplest and smallest movement that I could (ie not complicating the serve with excessive movements).

This morning I was reading Mock Roles, Not Objects and I was was reminded of my tennis experiences. It refreshed my thinking on the purpose and practices of TDD and mocking. There were several quotes from the paper that stood out for me:

• “Using TDD has many benefits but the most relevant is that it directs the programmer to think about the design of code from its intended use, rather from its implementation.”
• “Mock Objects changes the focus of TDD from thinking about the changes in state of an object to thinking about its interactions with other objects.”
• “Writing tests drives a framework to think about functionality. Mock Objects provides a framework for making assertions about those relationships and for simulating responses.”

Section 4 of the paper discusses Mock Objects in practices, detailing some guidelines for how to use them properly:

• “Only mock types you own”. Matt mentioned this one at our book club the other day in the context of skinning and wrapping APIs.
• “Don’t use getters”. Why? “Getters expose implementation, which increases the coupling between objects and allows responsibilities to be left in the wrong module. Avoiding getters forces an emphasis on object behaviour rather than state..”. I was taught not to use getters by Nick and I find myself discussing it often with each pair.
• “Specify as little as possible in a test”…”One of the risks with TDD is that tests become ‘brittle’, that is they fail when a programmer makes unrelated changes to application code.” This is on of the most common criticisms of TDD that I have heard from other developers. The paper goes onto say that the cause is that “…(the tests) have been over-specfied to check features that are an artefact of implementation, not an expression of some requirement in the object.” I’ve written tests that fall into in this category and I think it can be quite challenging to avoid doing so. Steve Freeman, one of the authors of the paper, blogged about this situation happening in a Coding Dojo earlier this year.

From time to time it’s useful to revisit papers like Mock Roles, Not Objects in order to remind yourself of the basics what you’re doing…. (and remember to watch the ball when playing tennis!)

Book Club: Working Effectively With Legacy Code – Chapters 14, 15 and 16 (Michael Feathers)

Mark has returned to the UK so I’m continuing in his tradition of posting about the Thursday technical book club

This week we continued going through Working Effectively With Legacy Code, tackling an unprecedented 3 chapters:

Chapter 14: Dependencies on Libraries Are Killing Me

In this chapter Feathers describes how an overuse of direct calls to libraries can cause pain. He suggests that wrapping the API can create a nice seam for testing, so long as there are no constraints imposed by the use of language features such as final/sealed. As Matt pointed out this ties into the following chapter where Feathers discusses when the skinning and wrapping approach is most appropriate.

Chapter 15: My Application Is All API Calls

Feathers presents two approaches for improving the landscape when the application is all API calls:

1. Skin and Wrap the API

• Feathers introduced this idea in chapter 10.
• It is very useful for creating a separation layer from third party libraries.
• As Ahrum pointed out this can sometimes be a difficult task if there is a proliferation of library calls in the API.
• Matt also noted that this approach has more of a method level granularity, where as responsibility-based extraction has more of a class/object level granularity.
• We had some discussion about how far to take the skinning and wrapping – for example would you skin and wrap the I/O classes? I’m not entirely sure where I’d draw the line but this does seem like something that could be pushed to an unnecessary extreme.

2. Responsibility-Based Extraction

Feathers says that this approach is better suited to more complicated APIs. He writes:

“When we have a system that looks like nothing but API calls, it helps to imagine that it is one big object and then apply the responsibility-separation heuristics…. We might not be able to move toward a better design immediately, but just the act of identifying the responsibilities can make it easier to made better decisions as we move forward.”

Chapter 16: I Don’t Understand The Code Well Enough To Change It

In this chapter Feathers details some practices that can be used to get a better understanding of the code, including:

• Sketching out important things that you find in the code and connecting the concepts as you understand them.
• Effect sketching
• Printing the code out and marking it up in different colours to indicate different responsibilities. I’ve not done this before but it may be useful when doing responsibility-based extraction.

Feathers also says that you should delete unused code. As Anita quite rightly pointed out, deleting unused code extends beyond just commented out code and can be hard to detect. For example, there may be if-condition blocks that will never called in the application, but may be unit tested as though they are.

Nested Diagnostic Contexts

On my current project we have what I think is a very well thought out logging strategy. We generate a number of logs that serve a variety of different purposes. One thing that is common across all of them is that they are configured to print information from Nested Diagnostic Contexts (NDCs). An “NDC is an object that Log4j manages per thread as a stack of contextual information.” There is a very clear and short example of using an NDC and the output it can produce here.

In our application we use the NDC class in lots of places. For example we have a LoggingContextInterceptor that creates and destroys an NDC for our controller methods:

public class LoggingContextInterceptor extends HandlerInterceptorAdapter {

    private final UrlPathHelper helper = new UrlPathHelper();

    @Override
    public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler)
            throws Exception {

        NDC.push(helper.getLookupPathForRequest(request));
        return true;
    }

    @Override
    public void afterCompletion(HttpServletRequest request, HttpServletResponse response, Object handler, Exception ex)
            throws Exception {

        NDC.clear();
        NDC.remove();
    }
}

Note that in the afterCompletion method above we are calling NDC.remove as well as NDC.clear. According to the API documentation the NDC clear method will empty and context information. The documentation also states that you need to call the remove method when exiting the thread because “this ensures that the memory used by the thread can be freed by the Java garbage collector”.

Inside our controllers we may push and pop additional information on and off the NDC, as well as log various events. A typical controller method in our app looks a bit like this:

    @RequestMapping(value = "/member/{memberNumber}/foostrategy", method = RequestMethod.PUT)
    public View updateFoo(@PathVariable("memberNumber") String memberNumber,
                          InputStream requestInputStream) throws IOException {

        // some processing

        logger.info("Updating " + strategy);
        service.update(strategy, fundCode);

        return StatusCodeView.ok();
    }

The logged message from this controller includes the NDC info:
2009-12-02 11:00:50,806 INFO [15554445@qtp-10481519-2 PUT /member/03003522/foostrategy] [FooController] – Updating FooStrategy[memberNumber=1234....]

Where the configuration for the Appender is:
log4j.appender.myAppender.layout.ConversionPattern=%d{ISO8601} %-5p [%t %x] [%c{1}] – %m%n

Logging with NDCs has proven to be very useful for helping the team understand and reproduce errors very quickly.

Passing by Name in Scala

Last week I wrote a little Scala application on a sunny Sunday afternoon. My aim was to improve my beginner level Scala skills, including my knowledge of the language and the testing tools available.

I hit a snag on one unit test and it took the minds of Ben and Mark to help unravel the mystery…. This is the test:

import org.scalatest.matchers.ShouldMatchers
import org.scalatest.Spec

class RobotSpec extends Spec with ShouldMatchers {
  describe("A robot") {

    //other tests omitted

    it("should be able to turn to the right multiple times") {
      val robot = new Robot(1, 2, North)
      robot.turnRight
      robot.toString should equal("1 2 E")
      robot.turnRight
      robot.toString should equal("1 2 S")
    }
  }
}

Where:

class Robot(var xPosition: Int, var yPosition: Int, var direction: Direction) {
  def turnRight(): Unit = {
    direction = direction.getRightDirection;
  }

  //other methods omitted

  override def toString(): String = {
    "%d %d %s".format(xPosition, yPosition, direction.abbreviation)
  }
}

sealed abstract case class Direction(abbreviation: Char, leftDirection: Direction, rightDirection: Direction) {
  def getLeftDirection: Direction = leftDirection
  def getRightDirection: Direction = rightDirection
}
case object North extends Direction('N', West, East)
case object West extends Direction('W', South, North)
case object East extends Direction('E', North, South)
case object South extends Direction('S', East, West)

Interestingly this test fails with a Null Pointer Exception when we try to turnRight the second time. Printing out some information about the robot’s direction variable inside the turnRight method gives some clues:

On the first call to turnRight:
“direction: North left: West right: East”

On the second call to turnRight:
“direction: East left: null right: null”

Why are the left and right directions for East null? You can see that each Direction case object has a dependency on two other Direction case objects, meaning that we have circular dependencies. In order to evaluate the left direction of East we need North, which in turn references East. What’s interesting for me is how null has been used when to break the circular dependency and allow compilation to succeed.

So how did we get the test passing? The answer was to use pass by name (or call by name):

sealed abstract class Direction(abbreviation: Char, leftDirection: () => Direction, rightDirection: () => Direction) {
  def getAbbreviation: Char = abbreviation
  def getLeftDirection: Direction = leftDirection()
  def getRightDirection: Direction = rightDirection()
}
case object North extends Direction('N', () => West, () => East)
case object West extends Direction('W', () => South, () => North)
case object East extends Direction('E', () => North, () => South)
case object South extends Direction('S', () => East, () => West)

Now the left and right directions are evaluated only when they are used in the turnRight and turnLeft methods.