Duck Typing vs Casting

In our current project a user may have groups of videos. These can be what we call system groups or user groups. An example of a system group is one that contains all videos received by a user today. The distinction is that whilst a user can manage a group they have created themselves (i.e. they choose what goes into it) they cannot manage a group that is created by the system.

When we get to the UI we find that we have a button that puts a group into edit mode. It doesn't make sense that this button is enabled for system groups, as they cannot be edited. So, when the button is rendered, it makes sense to decide whether or not it should be enabled.

Some folks might be tempted to set the enabled state of their button based on the type of the group, e.g.:

    if(model.getSelectedGroup().getClass().equals(SystemGroup.class)) {
        manageGroupLink.setEnabled(false);
    } else {
        manageGroupLink.setEnabled(true);
    }

I'm not really a fan of this approach. To me this says system groups can't be managed. And it says it in the UI code. To reiterate, this approach puts a business rule (system groups can't be managed) into the UI code. It also puts knowledge of the subtype hierachy into the UI which means that any careful work we might have done to ensure that the UI works with interfaces is now lost. But it is quick and easy. And if business rules change and more subtypes are created, maybe another developer will be the lucky one who has to fix things.

Actually, its perhaps hard to see in an isolated example why this is a problem. But as systems grow and this logic is required in more places it becomes a maintenance nightmare to have business rules repeated all over the UI that need updating if they change or if new subtypes are introduced.

Separating concerns into discrete layers is a fairly well accepted but often misused concept. The UI should arguably not be concerned with business rules and concrete implementations. It should be concerned with displaying data and reacting to user input.

If we stick to this rule, even though not doing so doesn't present a problem at this stage, we have protected ourselves against future changes by design. If we get into the habit of keeping UI components free of business rules and knowledge of subtypes then we might not even realise that doing so in a particular situation saved us a world of pain.

Duck typing is one possible solution to the problem. Its based on the theory that 'if it quacks like a duck it must be a duck'. The premise is that if an object responds in a certain way then we can treat it accordingly. So how would things look different if we applied this approach?

        manageGroupLink.setEnabled(model.getSelectedGroup().isManageable());

This implies that the selected group implements isManageable. Any implementation of group can implement this property. This nicely puts the business logic into the domain object. We could look at a system group and see that it returns false when isManageable is called. In my opinion, there's no better place to put this information. UI components don't need to know about the subtype hierachy and the job is done in one line of code (imagine if there were ten subtypes, some of which were manageable and so of which weren't, we'd have a big if-else block).

Conflict in the Workplace

I'm doing some work for a strange little company. Its an unlikely assortment of folk from vastly different backgrounds. I've been trying to hammer home the importance of using what I consider to be best practices. I lean towards an agile style using test driven development, continuous integration and pair programming as well as lightweight UML for working out designs.

There was a section of the codebase which kept turning up as the source of bugs. It was a model in the MVC sense that was statically accessed through the session. Around half of the UI depended on this object but these various components expected it to behave in a different way. Essentially it was the core data that everything needed, but the view of it that they required differed. A set of clunky switches had evolved to prime the model to behave in a certain way. The bugs centered around the fact that the model was often left in an inconsistent state because it wasn't clear exactly how it should be poked and prodded to achieve the desired state.

It seemed clear to me that we needed to refactor. Firstly I realised we needed to have subclasses of our 'god-object' model to separate out the concerns that were proving difficult to manage from a single class. Secondly it was difficult to write tests against objects that depended upon the session. There was a great deal of test friction involved in setting up a mock session and our tests had to effectively start the application in order to run, just to instantiate the model object we were trying to test.

The model was accessed via a call to Model.get(), a static accessor that retrieved the instance from the session. I wanted to replace this with an actual instance obtained from a factory or provider. I also wanted any classes to depend upon an interface for the model, not the actual class.

In my first cut I created some subclasses of our model and moved the bits of implementation out of the existing lengthy if statement ridden implementation into appropriate classes. These classes still depended upon the static accessor, but at least we now had a facade that could give us classes that were concerned with discrete chunks of functionality.

The second job was to remove the static access. This proved more difficult than I thought. This was because rather than model dependent classes being passed their model in the constructor (we're using wicket) they were sometimes using the static accessor to obtain a different model from that which had been used during construction.

The aim was to make everything dependent on an interface which was extracted from the existing model. With static access removed we were starting to have classes that became more testable. By refactoring to use interfaces we were able to provide an alternative implementation. Dependent classes wouldn't care which implementation they were using as the interface hadn't changed.

Now, the title of this post is 'conflict in the workplace', and here's the thing. Last week I was pairing on this refactor, working through our application and providing new implementations. It was going well. I had two days holiday and when I came back on Monday I discovered that the developer I had been pairing with had dramatically changed the interfaces.

When I began this refactor I had planned out exactly what I thought we needed to do and presented it to the team. I had their support and welcomed their suggestions. This seemed to be in stark contrast to a developer making sweeping changes in my absence with no consultation.

I had been refactoring to provide a less bug prone implementation of our existing API. Suddenly someone had gone rogue and changed the API. Since these changes I've been working through the application updating everything that depended on the old API to work with the new one. Not everything that is required is in the new API so I find myself rewriting code that had been working to fit the new order.

During a heated debate yesterday, I complained that I felt I was now wasting my time fixing code that turned out to not be needed any more. His response was that it was because I hadn't bothered to learn the new API. It was at this point that I realised I was angry that the API had changed. To me it was important not to break compatibility of our interfaces. To do so had far reaching implications on the rest of the application. During all of our discussions we'd never decided the API was wrong. Now I was fixing the fallout which was the result of new interfaces.

I'm not against these sort of changes, there are always different ways of doing things. I did find it very difficult to accept that such vast changes were made by an individual in my absence with no discussion.

I'm not about to dictate that we rollback everything that has been done. People tend to be unhappy when they are told what to do. We will be able to fix things. Sometimes people need to make mistakes in order to learn. And the lessons learned this time I hope are that buggy implementations should be replaced by better implementations. Interface compatibility should not be broken. And decisions affecting the whole team should be taken by the whole team. Rant over.