Separation anxiety ?

We all have separation anxiety. Its a human tendency. We love inertia, and we don’t like change. But why should your code have separation anxiety ? Its not human (even though it might try and grow a brain of its own at times)!

I bring this up because I got the chance to work with someone who had some questions on how to test UI code. Fairly innocuous question, and in most cases, my response would have been, keep the UI code simple and free of any logic, and don’t worry too much about it. Or you could write some nice end to end / integration tests / browser based tests. So with that response set in mind, I set off into the unknown. Little was I to know was that was the least of my concerns. As I sat down to look at the code, I saw that there were already tests for the code. I was optimistic now, I mean, how bad could things be if there are already tests for it ?

Well, I should remember next time to actually look at the tests first. But anyways, there were tests, so I was curious what kinds of tests they wanted to write and what kind of help I could provide, if any. So it turns out, the class was some sort of GUI Component, which basically had some fields, and depending on whether they were set of not, displayed them as widgets inside of it. So there were, say, 5 fields, and differing combinations of what was set would produce different output. The nice thing was that the rendered data was returned as a nice Java object, so you could easily assert on what was set, and get a handle on the widgets inside of it, etc. So the method was something along the following lines :

public RenderedWidgetGroup render() {
    RenderedWidgetGroup group =
        createWidgetGroup(this.componentId,
                          this.parent);
    if (this.name = null) {
        return group;
    } 
    group.addWidget(new TextWidget(this.name));
    group.addWidget(
        new DateWidget(
            this.updatedTimestamp == null ?
                 this.createdTimestamp : this.updatedTimestamp));
    return group;
}

So far, so good, right ? Nice, clean, should be easy to test if we can set up this component with the right fields. After that, it should just be a few tests based on the different code paths defined by the conditionals. Yeah, thats what I thought too. So, me, being the naive guy that I was, said, yeah, that  looks nice, should be easy to test. I don’t see the problem.

Well, then I was taken to the tests. The first thing I see is a huge test. Or atleast thats what I think it is. Then I read it more closely, and see its a private method. It seems to take in a bunch of fields (Fields with names that I distinctly remembered from just a while ago) and churn out a POJO (Plain Old Java Object). Except this POJO was not the GUI Component object I expected. So obviously, I was curious (and my testing sensors were starting to tingle).  So I followed through to where it was called (which wasn’t very hard, it was a private method) and lo and behold, my worst fears confirmed.

The POJO object generated by the private method was passed in to the constructor of the GUI component, which (on following it further down the rabbit hole) in its constructor did something like the following :

public MyGUIComponent(ComponentId id,
                      Component parent,
                      MyDataContainingPOJO pojo) {
    super(id, parent);
    readData(pojo); 
} 

And of course, readData, as you would expect, is a :

• Private method
• Looks through the POJO
• If it finds a field which is not null then it sets it in the Gui Component

And of course, without writing the exact opposite of this method in the unit test, it just wasn’t possible to write unit tests. And sudddenly, it became clear why they were having trouble unit testing their Gui Components. Because if they wanted to test render (Which was really their aim), they would have to set up this POJO with the correct fields (which of course, to make things more interesting / miserable, had sub POJOs with sub POJOs of their own. Yay!) to be exercised in their test.

The problem with this approach is two fold :

1. I need tests to ensure that the parsing and reading from the POJO logic is sound, and that it correctly sets up the GUI Component.
2. Every time I want to test the render logic, I end up testing (unintentionally, and definitely unwantedly) the parsing logic.

This also adds, as I saw, obviously complicated pre test setup logic which should not be required to test something completely different. This is a HUGE code smell. Unit testing a class should not be an arduous, painful task. It should be easy as setting up a POJO and testing a method. The minute you have to perform complicated setup to Unit test a class (Note, the keyword is unit test. You can have integration tests which need some non trivial setup.), stop! There is something wrong.

The problem here is that there is a mixing of concerns in the MyGuiComponent class. As it turns out, MyGuiComponent breaks a few fundamental rules of testability. One, it does work in the constructor. Two, it violates the law of demeter, which states that the class should ask for what it needs, not what it doesn’t need to get what it needs (Does that even make sense ?). Thirdly, it is mixing concerns. That is, it does too much. It knows both how to create itself as well as do the rendering logic. Let me break this down further :

Work in the constructor

If you scroll back up and look at the constructor for MyGuiComponent, you will see it calling readData(pojo). Now, the basic concept to test any class is that you have to create an instance of the class under test (unless it has static methods. Don’t get me started on that…). So now, every time you create an instance of MyGuiComponent, guess what ? readData(pojo) is going to get called. Every. Single. Time ! And it cannot be mocked out. Its a private method. And god forbid if you really didn’t care about the pojo and passed in a null. Guess what ? It most probably will blow up with a NullPointerException. So suddenly, that innocuous method in the constructor comes back to haunt you when you write yours tests (You are, aren’t you ?).

Law of Demeter

Furthermore, if you look at what readData(pojo) does, you would be even more concerned. At its base, MyGuiComponent only cares about 6 fields which it needs to render. Depending on the state of each of these fields, it adds widget. So why does the constructor ask for something totally unrelated ? This is a fundamental violation of the Law of Demeter. The Law of Demeter can be summed up as “Ask for what you need. If you need to go through one object to get what you need, you are breaking it.”. A much more fancier definition can be found on the web if you care, but the minute you see something like A.B().C() or something along those lines, there is a potential violation.

In this case, MyGuiComponent doesn’t really care about the POJO. It only cares about some fields in the POJO, which it then assigns to its own fields. But the constructor still asks for the POJO instead of directly asking for the fields. What this means is that instead of just creating an instance of MyGuiComponent with the required fields in my test, I now have to create the POJO with the required fields and pass that in instead of just setting it directly. Convoluted, anyone ?

Mixing Concerns

Finally, what could be considered an extension of the previous one, but deserves a rant of its own. What the fundamental problem with the above class is that it is mixing concerns. It knows both how to create itself as well as how to render itself once it has been created. And the way it has been coded enforces that the creation codepath is executed every single time. To provide an analogy for how ridiculous this is, it is like a a Car asking for an Engine Number and then using that number to create its own engine. Why the heck should a car have to know how to create its engine ? Or even a car itself ? Similarly, why should MyGuiComponent know how to create itself ? Which is exactly what is happening here.

Solution

Now that we had arrived at the root of the problem, we immediately went about trying to fix it. My immediate instinct was to pull out MyGuiComponent into the two classes that I was seeing. So we pulled out a MyGuiComponentFactory, which took up the responsibility of taking in the POJO and creating a GuiComponent out of it. Now this was independently testable. We also added a builder pattern to the MyGuiComponent, which the factory leveraged.

class MyGuiComponentFactory {
    MyGuiComponent createFromPojo(ComponentId id,
                                  Component parent,
                                  MyDataContainingPOJO pojo) {
      // Actual logic of converting from pojo to
      // MyGuiComponent using the builder pattern
     }
}
class MyGuiComponent {
    public MyGuiComponent(ComponentId id,
                          Component parent) {
        super(id, parent);
    }
    public MyGuiComponent setName(String name) {
        this.name = name;
        return this;
    }
} 

And now, suddenly (and expectedly, I would like to add), the constructor for MyGuiComponent becomse simple. There is no work in the constructor. The fields are set up through setters and the builder pattern. So now, we started writing the unit tests for the render methods. It took about a single line of setup to instantiate MyGuiComponent, and we could test the render method in isolation now. Hallelujah!!

Disclaimer :
No code was harmed / abused in the course of the above blog post. There were no separation issues whatsoever in the end, it was a clean mutual break!

My code’s untestable !!

I have frequently heard this complaint, from some really great engineers in the past year. My code’s untestable, there’s no way i can test this, the only way to test my code is to write a full on end to end test. And in some cases, it was actually true. But the thing is, it doesn’t need to be that way. There are always ways to twist and turn the untestable code into testable code, in other words refactor.

But before you get excited, and go aha, thats what I am going to do, hold your horses for a second. What are you going to go refactor ? Well, don’t scratch your heads, there are a few things to look out for. Similar to the code smells we had, there are similar smells we can look for, which indicate that the code is untestable. And there are a few standard ways to tackle them, and make your classes testable. So without further adieu, lets take a look at the more common and annoying testability smells.

1. Constructor doing work :
   This is one of the biggest things preventing a class from being testable. There are many names to this smell, including constructor doing work,  Breaking the law of demeter, etc. but all it comes down to is the constructor doing more than just assigning stuff to local variables. For example, something like :

   XPathConvertor() {
   this.xpathDatabase = XPathDatabaseFactory.getDatabase();
   XPathMapper mapper = new SimpleXPathMapper(“Simple Mapper”);
   this.xpathTranslator = XPathTranslatorFactory.getTranslator(this.xpathDatabase, mapper);
   }

   While the above may seem a contrived example, or too simple, it exhibits what is at the centre of most bad constructors. One, its a default constructor. Then it goes out, grabs a database out from ether (Static factory method call), and then creates a mapper, and then passes those two to get a XpathTranslator object. Now, take my word for it, but the XPathConvertor only needs xpathTranslator. SO what is it doing with the darn database and the mapper. This is breaking the law of demeter, which states that “The constructor should only ask for what it needs, and nothing else.”

   Why is this bad ? Well, for one, if the thing your constructor is creating is a heavy piece of service like a database or something, there’s a huge hit in your test. Your test is no longer a unit test, but an integration test. Each call now has to travel to the DB and back, and just makes everything slower. Secondly, there are some cases in which it picks up a service which you just can’t work with in a test. Something which either needs the whole production setup, or just doesn’t work in a unit test. And since it reaches into static factories to get that, there’s no way for you to slip in your mock.

   So instead, start passing in what is needed to your constructor. This forms the basis of Dependency Injection, or slipping in a fake, or whatever you want to call it. Basically, your constructor takes in what it needs, and all it does is assign stuff to local variables. No work is done there. So the above code becomes something like :

   XPathConvertor(XPathTranslator translator) {
   this.xpathTranslator = translator;
   }

   So much cleaner, and only has what it needs. So in our test, you can create a translator and pass it in which uses a mock db, or pass in a fake translator or whatever. The point is, testing becomes easier.

2. Global State :
   The second biggest complaint with untestable code. It usually has to do with Global state. Or as some people like to call it, putting things in and pulling things out of ether. This might be anything from using global static singletons to static method calls inbetween your method.

   How is this bad ? Consider some method you are testing. What if it suddenly reaches out into the ether, and grabs some object and uses it to perform its calculations. You say, ok, I can somehow add a setter which allows me to set its state. Now what if there are multiple tests running in parallel…. Yes, exactly. Not good. Furthermore, you can’t mock a static method, which makes life miserable.

   Consider this question then, why does it need to be static ? What benefit are you getting, other than the fact that you don’t need to create an object ? Is it really worth making code untestable in order to reduce one line of code of creating an object ? The answer 9 times out of 10, I find, is no.

Your code smells!!!

I was planning to talk about Dependency Injection or the difference between Mocks, Stubs and Fakes, but I think I would prefer to get this one out first. For those of you who haven’t heard the term before, a code smell is something that indicates there is something wrong with the source code, be they design problems or signs that the code needs refactoring. So in this post, I would like to mention a few common code smells, their identifying patterns and how to fix them. So without any further delay, lets start :

1. Too many comments
   The Problem : Lets start with the easiest to identify code smell. Comments are good, if they are describing a class or a method. But when you start having comments in your code explaining what a particular block of code does, you know you are in trouble.
   The Fix : If there is a block of code which does some complicated stuff, and you feel you need to comment it to make it easily understood, pull it out into a well named method, and let the method name describe what it does.
   .
2. Long Class / Method
   The Problem : These two code smells actually are similar. If you have a method which goes beyond 10 – 15 lines, then you have a long method. While there is nothing technically wrong with long methods, its not as easy to comprehend as a nice small method, and can make maintenance a pain in the rear. Long classes also are similar, having too many things that its trying to do. If you generally try describing a class and have to use ands and ors, then you have a long class.
   The Fix : Pull out parts of the long method into smaller well named methods. The advantage is two fold. One, your method is much more readable now. And two, you can now test individual methods you have pulled out, making testing a much easier task than one giant method. Same with classes, break it up into multiple smaller easily testable classes.
   .
3. Primitive Obsession
   The Problem : How many times have you had to write a class which takes in, say a phone number ? And how many times have you passed in a String or a long to the class which asks for the phone number ? If you raised your hand, then congrats, you have a code smell known as primitive obsession. This happens when instead of creating an object, you pass around primitives, and write functions to operate and convert that primitive from one form to another. So you might have to create utility classes which give you a phone number with brackets from a string, and so on and so forth.
   The Fix : Just give the poor thing a class. If you operate on phone numbers, create a PhoneNumber class which has methods to operate on the number. Makes it easier for anyone using the class as well, and of course, its testable :D.
   .
4. Feature Envy / Inappropriate Intimacy
   The Problem : Feature Envy is when a method on a class is more interested in other classes than the class to which it belongs. The reason for this could be as simple as the method being in the wrong class to something more non trivial. Inappropriate intimacy is when two classes are so tightly coupled that one depends on the other to work a particular way for it to work.
   The Fix : For feature envy, just move the method to where it belongs. If it does work solely on some other class, then maybe it belongs on that class instead of where it is currently. For inappropriate intimacy, you need to figure out if the problem is something simple or something more complicated. It might be that the interfaces weren’t selected appropriately, or you might need to introduce a layer to keep the coupling loose, or even refactor the code to make sure it does not depend on another tightly coupled class.
   .
5. Lazy class / Dead code / Duplicate code / Speculative Generality
   The Problem : All of the above are usually simple code smells which indicate you have code which you don’t need. Lazy class is when a class doesn’t do enough to justify its existence. Dead code is obvious, code which is not used or dead. Duplicate code, duh, Its duplicated. Speculative generality is the most interesting of the lot. Its when you write some code for something which you don’t need yet, but may need at some point in the future.
   The Fix : For the first three code smells, the fix is trivial. Delete it. Dont even think about it, just blindly delete it. Duplicate code is a pain to maintain as well, pull it out into a method and then delete the duplicates. Speculative generality is one thing people don’t realize they are doing or feel they need to do it now since otherwise, it might become difficult to do in the future. The interesting thing is that the feature they added speculatively is rarely ever used. Its an additional overhead to maintain and test for something you never use. Don’t do it. If you can implement it now, you can implement it when you need it. Just delete that darn code.

There are a lot more code smells than I could list out here, but these are a few of the most common ones you should keep a lookout for. Google search Code smells if you want to learn more about these insidious creations :D. Next time, before I start dependency injection, I think I will rant about things which make code untestable. So in a sense, Testability code smells.

Mock, Mock! Who’s there ?

The concept of mocking is not a new idea, but its one that has been gaining traction recently. But still, whenever I tell people to mock out their dependencies when they are trying to test, they look at me as if wondering what the heck I’m smoking. Well, if I was smoking something, I would share it. But mocking is a great thing for writing small unit tests.

I get usually one of the two statements / questions when I tell someone to just mock something out.

1. Well, if I am mocking things out, then I am not really testing it, am I ? OR I don’t want to mock things out. I need to test the method’s interaction with other classes.
2. What about the mocked class ? We have to make sure it works too.

Well, to number 1, I say, if by mocking things out, you have nothing but a series of expected calls and returns, and there’s no actual class specific behavior there, then does that Class really deserve to be a class ?

And the second part of number 1, well…. Is that really a unit test then ? Ideally, you should have a lot of unittests to make sure the class specific logic is sound, and then a few integration tests to make sure that everything is hooked up correctly.

And number 2… This is where you go and write unit tests for the class you have just mocked. Don’t depend on large scale integration tests to test all your classes. Write nice small unit tests which are fast and precise. The larger a test gets, the harder it is to find why a particular test broke and how.

So hopefully by now, I have you convinced that mocking may not be all that bad. Fine, you say, so how do I go about this mocking thing ? Glad you asked. While there are many mocking frameworks out there, I am going to just talk about EasyMock for Java. JMock is very similar and could be pretty interchangable.

The first requirement before you can use any mocking framework is the ability to inject mocks into your class. This is Dependency Injection at its core, without which you will have to find workarounds like protected setter methods and the like. But basically, if a class uses some Service or Database, make sure that you can override it with a Mock Database by passing it into a constructor or setting it via a method.

Once thats done, the first step in using a mock is creating the mock object. In EasyMock, its as simple as :

MyClass myObject = EasyMock.createMock(MyClass.class);

Thats it, nothing fancier than that. It helps if MyClass is an interface, but I believe EasyMock supports mocking non interface classes as well. Once you have done that, you can inject this mock object into the classes which you are testing.

The next step is setting expectations. For void methods, it as simple as just calling the method with the expected parameter. For example :

myObject.myVoidMethod(“ThisStringWillBePassed”);
EasyMock.expect(myObject.methodWithReturnValue(“ExpectedArg”)).andReturn(“ReturnValue”);
EasyMock.replay(myObject);

The EasyMock.replay(myObject) tells EasyMock that you are done setting expectations and that the next time a method on the object is called, treat it as an actual call. So then in your test, you proceed as normal invoking the methods you care about, and then finally, you call :

EasyMock.verify(myObject);

This ensures that all the expectations set on myObject were met. Now EasyMock also supports additional features like setting expectations on number of method calls, throwing exceptions, flexible argument matchers and so much more. For more information, check out the EasyMock home page.

Now a few caveats with regards to mocking. It is very easy to degenerate some tests into what we call a mockery, where we end up testing mocks and their interaction instead of the actual class we want to test. So Don’t overuse mocks. Use them when you have to test something which depends on Database or expensive service calls. Also if your test ends up exercising a bunch of mock calls and nothing else, that might be a hint that your class really does not belong. And of course, it goes without saying that don’t mock the class you are testing.

Also, don’t set up Mock layers where a class which indirectly depends on some service object uses the mock layer. You should always mock the classes which your Class Under Test directly depends on, and not classes which it indirectly depends on. And sometimes, a mock might not be what you are looking for. Instead, a simple Stub or a Fake might be more useful. I might talk more on this or Dependency Injection in my next post.

The Testability Explorer cometh….

So last time, we explored on how to find hotspots and untested code in your code base. But then you start looking at your code, and then you realize there is a reason why you didn’t test the darn thing. The code’s untestable. Whoo hoo..

Well, there is no such thing as untestable code. Or rather, all untestable code can be refactored to make it much nicer and easier to test, through a variety of techniques. The first and foremost reason for untestable code ends up being “Constructor doing work.” If the constructor of a class does anything more than stuff like “this.x = x” or if it tries to call a constructor itself or use a *GASP* static factory, bingo, you have a problem.

But fixing that isn’t the target of this post. That will be covered in a later one, cause its a doozie. No, in this post, I want to talk about how to find these untestable code snippets without any manual effort. Every code base has atleast a few of these gems, which turn up being a nightmare to test, and in turn make everything depending on it a nightmare as well. Well, fear not, for the Testability Explorer cometh…

The Testability Explorer (http://www.testabilityexplorer.org) is an open source tool which looks at classes and does cyclomatic complexity analysis on it. What does that mean ? Well, it looks for things which make testing hard, like conditionals, and recursively dives in to objects it instantiates to find their testability score. In that respect, it is a static and recursive analysis of a code base. It takes all these into consideration and assigns a score to each class. Based on these scores, a class is either

• An excellent to test class
• A Good class but could use some work
• A horrible class to test, needs a lot of work.

The following image, taken from the Testability Explorer website, shows a sample report generated by the tool :

As you can see, it generates html reports with bar graphs and pie charts. It can even, depending on the options you specify, allow you to dig in deep into the problem classes and find the method and line which causes you the most problem to test. This can give you great insight on deciding what classes need refactoring first to make it testable. A lot of times, fixing the most problematic one causes a ripple effect, which fixes a bunch of problems in classes depending on it.

Another great thing is that the Testability Explorer can take jar files, so if you don’t want to expose your code directly to it, you have an option. Though sadly, the Testability Explorer is currently only available for Java code. It stands to reason that something similar could be done for C++, though you Javascript guys are out on your own.

All in all, a great tool. But don’t depend solely on it. It is great as one tool in a repertoire of tools, but not just by itself. Testability Explorer is also a great way to notice trends, of whether your code is growing more testable or untestable, and other great things, just like code coverage. Though leading you to nothing more than testable code, you would be surprised at how much positive impact increased testability and tests can have on the quality of a project.

So go check it out. And enjoy.