Code as you would order a burger

Recently during my work, I came across some code where the paradigm Tell Don't Ask came into play. You sometimes see code written that's not really using OO to its full strengths and still is of a procedural stance. There's often frequent examples where an object asks other objects for data and then does something with it. This is not really OO even though you may still be using other aspects of OO such as encapsulation, inheritance or polymorphism. It's still asking for data whereas the real value-add for OO is message passing or the behaviour of the objects rather than the data they may contain. I've come to understand this implicitly via the usage of mocks during testing. The use of mocks results in code that better follows the 'Tell, Don't Ask paradigm. One of the originators of mock objects noted that:

The trigger for our original discovery of the technique was when John Nolan set the challenge of writing code without getters. Getters expose implementation, which increases coupling between objects and allows responsibilities to be left in the wrong module. Avoiding getters forces an emphasis on object behaviour, rather than state, which is one of the characteristics of Responsibility- Driven Design - Tim McKinnon

Once I came over this mental hurdle, I found I wasn't primarily concerned with the data passed between the objects but really that the appropriate methods calls or methods (messages) between objects were observed. I used the Mockito mocking library and I find the most of my tests are interaction tests i.e. verifying that methods calls were made in the manner expected. State-based testing is still important but at the right place. This got me thinking about objects in terms of Alan Kay's emphasis on message passing http://c2.com/cgi/wiki?AlanKaysDefinitionOfObjectOriented. The following examples illustrate why telling an object to do something rather than asking it for its data is better.

I work a lot with enterprise integration and one of the examples that illustrates this point is message concatenation. During conversion from an internal message to one that that is supported by an external protocol. Dependent on whether a message is concatenated or not, then the message may be decorated with extra parameters to indicate that it is a multipart message rather than a single message.

In this example, the extra metadata parameters that get added are:

• The message reference for the multipart message
• The part number of the message
• Total number of parts for the message

Implementation using "Ask" approach
A factory was used to create a map of parameters for message concatenation

Loading ....

The protocol converter that used this information was coded like this:

Loading ....

This bad for several reasons:

• The MultipartMessageParameterFactory returns an abstract datatype. In this case a map, so users of this class have to know what keys to pull out the data. If this approach really must be followed, a domain object to hold the multipart parameters would be better, maybe a simple POJO with accessors for the different parameters.
• The main problem with this approach is it results in Inappropriate Intimacy. The user is asking for some data and then assembling that data thereafter. The lookup keys for the map are made public presumably to mistakenly enforce DRY principles for magic strings within a unit test.
• This class could become better by passing in the parameter factory in the constructor so it can be mocked. In this way, the unit test for this class will be simpler although it remains too complicated.

Implementation using "Tell" approach

The Tell is all about telling an object to perform some piece of work on your behalf rather than asking that object for data and then doing the work yourself. In the previous example, the protocol converter asked another object for the message concatenation info and then added those parts itself to the message. In the following example, we ask another object to do this bit of work for us. Firstly we introduce a domain object to replace the map use for the multipart parameters:

Loading ....

The factory now returns this POJO instead of the naked ADT:

Loading ....

Now the converter asks another object, MultiPartMessageUpdater, to do the message updating:

Loading ....

The code for updating the message is pushed into its own class. As well as enforcing the single responsibility principle, it also endorses the command and query separation principle so that the retrieval of the actual parameters (query) is separated from the addition of those parameters to the message itself (command).

Loading ....

As a byproduct, the test for the converter becomes very simple. Beforehand we would have to test every permutation of concatenated parameters. Now because code has been pushed to their logical place the test boils down to just testing the following behaviour:

• Message should be updated if message is part of a multipart message
• Message should be untouched if message is just a single message

Loading ....

The permutations for those 3 message parameters are pushed to where it should be namely the test for the MultipartMessageParameterFactory. Each test then tests what it should instead of testing the unit and all of its dependencies' behaviour at the same time resulting in a combinatorial explosion of test methods making the test unclear.

Conclusion

• We've now got a class which now asks other classes do work i.e. operation requests instead of asking for data and doing the work ourselves.
• By following this approach the single responsibility principle naturally falls out. Each class performs a single well, understood operation and only that operation. This makes those classes loosely coupled and become candidates for reuse in different contexts if needed.
• Unit tests have become clearer and focused as they only testing a single object rather than a multitude. If you notice I've endeavoured to use constructor injection for any dependencies. This makes the testing of classes easier as I can mock out any external dependencies.

I remember this paradigm by thinking what you would do if you ordered a burger in McDonalds. Would you ask the server for two sliced bun halves, some mayo, lettuce, a hamburger, cheese and tomato sauce or would you just ask for a Big Mac? :D
Links

• Mock Roles not objects by Tim Mckinnon, Joe Walnes, Steve Freeman and Nat Pryce
• Alan Kay's message to the Squeak mailing list about OOP, inheritance, and message passing.
• Growing Object-Oriented Software by Steve Freeman and Nat Pryce

JUnit Theories to the Rescue

I recently developed a restart manager to respond to JMS connection outages. After the connection was restored, I needed to reinitialize my message clients in order for them to rebuild their JMS sessions. But I needed to test that a restart only occurred for particular status transitions. At first I thought this would be an onerous task as I would have to work out all the status permutations. That is until I came across JUnit Theories.

JUnit Theories
Theories allows one to write tests that apply to a (potentially infinite) set of data points rather than having to recreate the same test multiple times with different data or creating one test and iterating through your own collection of data values.
Here is an example:

Messaging client
First off is the interface for the messaging client. It has two lifecycle methods, start and stop which are self-explanatory, as well as methods for reading and sending a message which for brevity are just strings.
Status Enum
The following enum represents the status of the JMS connection. As can be seen there are numerous states so it would be cumbersome to work out all the possible combinations for each status transition. This is where Theories become so useful. More of that later.

Restart Manager
This class is used to restart a message client when it recognizes a status transition from FAILED to STARTED.
Test class
Finally here's the test class. Notice I'm using mock objects here using Mockito. As I'm programming to the interface of the messaging client not its implementation, I can make use of a mock and verify its behaviour.  Presently as of JUnit 4.8, theories is still within an experimental package.
The most important bit of configuration is @Datapoints annotation. This sets up the data that'll be pass into the test methods. In this test I have two theory methods. One theory checks that the message client is restarted when there is a suitable status transition i.e. FAILED to STARTED. Another theory checks that the message client is not restarted if the status transition is not suitable. i.e. not FAILED to STARTED.
When you run the tests you'll find the all the combinations of the Status enums are passed to the test methods in question as shown below:
In this way, I ensure that all possible transitions are covered by my unit tests and the restart manager's behaviour is as required.
Conclusion
The use of theories allows tests to be devised that cover all possible combinations of data. This is in contrast to parameterized tests where the dataset to be passed to a test is strictly defined and the onus is on the developer to work out what data is needed for a particular range of tests. Each approach can be used in different situations as dictated by your requirements.

Programming Retrospective

I've recently had to develop and improve a legacy code base. This has reminded me of a several anti-patterns one should consider when coding. This is by no-means an exhaustive list but conveys some of the most frequent ones in my experience.

Final classes without interfaces

There were a lot of instances where classes were declared as final. This may be a good thing from a security perspective but from a testing perspective it meant that mock classes could not be created resulting in unit tests that were harder to write or could not be written.

There are tools to can create mock objects even without an interface. A particular favourite of mine is Mockito. In fact the mocking of final classes can be overcome through the use of the PowerMock API with Mockito.  However a class with an interface is preferable from an OO standpoint as the use of concrete classes results in tighter coupling between participants.

Lack of Defensive Programming

There are numerous examples of methods without any defensive checks. The caller of this method had a try catch block to catch a NullPointerException, NPE. This is a bad code smell because the onus is on the caller of the method to assert the validity of parameters that are passed to the method in question. This is the wrong place. From an encapsulation point of view, it should be the method that validates whether its input parameters are valid or not. This is a form of Design by Contract where the method checks to see if input parameters satisfies some condition and reacts accordingly. I like to think this as the 'Bouncer' Pattern. If you don't look right, you're not getting in :).

A contrived example is shown below:


This short of approach should also be applied to constructors to ensure the object has a valid state and is fully built before use. Furthermore the throwing of exceptions give more context as to what has caused the failure rather than catching a NPE and having to retrospectively determine what has called that NPE to be thrown.

A favourite API of mine is the Validator class in Commons Lang which lists numerous methods for validation in different contexts.

Exposure of super state to child classes

I've seen a lot of instances where child classes would use the state variables of a super class. Variables had protected visibility so there was naked access to these variables which are inherently dangerous. This is because a child class could change the reference to a super class variable with unforeseen consequences.  I think the intention was to provide access to a child class with a parent's state so the child class can perform an operation. A quick fix would be to only allow these parent state variables to be access through accessor methods.

http://c2.com/cgi/wiki?InappropriateIntimacy


But this begs the question on why child classes needed that sort of access in the first place. IMHO, private state should never escape the confines of a class, only behaviour.

The other smell was that the super class was becoming top heavy with functionality. Probably the reason was that common functionality was pushed up the class hierarchy for re-use by child classes. But this resulted in the super class becoming bloated and unfocused. A better solution would be to use delegation techniques rather than inheritance.

http://www.refactoring.com/catalog/replaceInheritanceWithDelegation.html

When I look at a class functionality, I like to keep in mind a Unix philosophy i.e. Do one thing but do it well. If you find your class not adhering to that maxim, that's a sign refactoring is in order.

Printing out error messages to console instead of logging

I've came across a few situations where exception stack traces were dumped to console. Don't do that. Use a logging framework such as log4j. If log4j is used then a console appender can be used to achieve the same goal. Furthermore errors and warnings could be logged to a specific destination i.e. a file so one can see only pertinent errors and not worry about debug messages.

Another observation was the lack of categories used in logging. Most of the statements I saw were a generic dump of error messages for the whole platform. Using categories allows log messages to be sorted. For example I could have a category called com.acme.X for X related logs and com.acme.Y for Y related logs. If I wanted to see all logs I create another appender that logged at the com.acme level.  At the very least, use the fully qualified name of the class the logger resides in as the logging category. The use of categories results in greater capabilities on what should be logged and where it should be logged to. In this example X and Y are logged to different appenders but the possibilities are endless depending on how the categories are devised.

Classes with unclear focus

There were examples of classes trying to do too much. An example would be a Handler class. The main functions are listed below

• Setup relevant properties needed by the handler
• Handle requests.
• Convert a request to a protocol specific message.
• Handle synchronous and asynchronous responses

This resulted in a class of over 1000 lines with several private methods.  This was one of the major smells in that many private methods were only working on a subset of the private fields of the class. This implies that the class is trying to do too much.

Most of the examples I've seen are that the majority of functionality is realized inside one class instead of being delegated to other classes. The lack of delegation means the intention of the class is lost. Furthermore testing of the handler becomes more problematic. By delegation, each of these functions can be tested in isolation.

http://c2.com/cgi/wiki?LongMethodSmell
http://c2.com/cgi/wiki?GodClass

Unwieldy or unneeded comments

There were a lot of instances where code comments were of no use or didn't add extra information. For example, one method contained a lot of retrievals from a database along with ambiguous looping constructs. Each part of the method contained a comment explain what the next section of code would do. The reason I don't like this firstly comments are deodorant on 'smelly' code. That comment is probably there because the code is not clear enough to be understood. Secondly comments are brittle. If I changed that section of code, then I have to remember to change the comments, another piece of maintenance.

I am a proponent of 'Programming by Intention'. This is a programming style where you give meaningful names to methods, variables, classes etc so that the intent of the object in question is clear. Dave Astel gives an excellent overview here: http://www.informit.com/articles/article.aspx?p=357688

In the case of the Handler class, the method was essentially doing three things:

• Obtaining a customer ID
• Obtaining an billing ID
• Obtaining other parameters from a database and checking to see if those parameters had values.

This resulted in a method of  200+ lines. The clarity or intention of the method has been lost. To regain the intention of this method, the method should be functionally decomposed into smaller methods.

i.e.


Now the intention of this method is clearer. The code becomes self-describing and there is not need for extraneous comments.

'Programming by Intention' is not used to declare all commenting is bad, just that commenting must not duplicate a purpose. If the code is clear then commenting what the code does is unneeded. However comments may still be needed. You could draw attention to a particular algorithm being used i.e MergeSort or that the code fixes a particular defect. When the comment has value it should be included, if not it should be discarded.

Use of exceptions to control program flow

Simple. Don't to it. The following link provides arguments:

http://c2.com/cgi/wiki?DontUseExceptionsForFlowControl

Throwing of ambiguous exceptions

There were numerous occasions where java.lang.Exception was thrown instead of a more specific exception. This is bad practice because throwing an ambiguous exception means the catcher cannot react to the exception in different ways. An ambiguous exception loses information on whether the situation is recoverable or irrecoverable. The meaning of the error is also lost. A specific exception should be thrown for a particular situation.

http://c2.com/cgi/wiki?ExceptionPatterns

Use parameter objects instead of long method signatures.

There were a few cases where methods had long method signatures. I'm talking about 10 or more parameters. This makes the method call unwieldy and prone to mistakes. A better approach is to use a parameter object which encapsulates the method signature, simplifying the method call.

http://c2.com/cgi/wiki?ParameterObject

Furthermore different parameter objects can be used to group together related parameters for different contexts. This is preferable than nullifying unneeded parameters in the long method signature.

Never Duplicate Code

http://c2.com/cgi/wiki?OnceAndOnlyOnce

A few examples were observed where code was duplicated and in some places just a copy and paste job.  This means that if any defects are found, then the fixes have to be applied in more than one place. Ideally code should be written using DRY principles i.e. Don't Repeat Yourself. Situations where the same code exists in different places should be remedied by that code being pulled out into a separate method and re-used.

Return nulls from methods.

There were a few cases where a call to a method resulted in a null being returned. For instance a client asked for a map and got returned a null because the input parameters were incorrect. The onus is then on the callee to check the result is not null before using the result. A better approach would be to return a NULL object. The Null Object pattern provides an alternative. It connotes the absence of an object. Instead of using null, the Null Object pattern uses a reference to an object that doesn’t do anything.

http://en.wikipedia.org/wiki/Null_Object_pattern

In this example, instead of returning null, an empty map should be returned. The client then doesn't have to check for nulls. This leads to safer code.

Conclusion

A lot of my recommendations are based on Martin Fowlers' Refactoring which gives guidance on how to remove particular code smells. However as legacy code is usually not particularly amenable to unit testing, refactoring can give a low confidence level as there are not the number of unit tests to back it up. Part of the Test Driven Design (TDD) approach is that unit tests are written to prove the behaviour of the system at a granular level. Once you have the tests, you have the confidence to refactor as you can regression test to see the system behaves as before. In my opinion a lot of the defects seen in production code would be diminished by the use of unit testing and paying heed to the aforementioned anti-patterns.

Recommended Reading

• Refactoring: Improving the Design of Existing Code by Martin Fowler. The classic book on taking poorly designed code and making it better without breaking it.
• A good introduction to TDD. Test-Driven Development: A Practical Guide by Dave Astels
• A TDD book for the more seasoned developer: Growing Object-Orientated Software, Guided by Tests by Steve Freeman and Nat Pryce. Probably if TDD was adopted in the first place many of these problems I described would not have arisen in the first place.
• But if you have a legacy code base showing these symptoms then a great book to read is Working Effectively with Legacy Code by Martin Feather. It has a set of techniques for changing code so that it is more suitable to be covered by unit tests