2012-05-30

Testable C++ and getters in interfaces

There are written plentiful about the evils of getters and setters in objects, with the simplest - and most extreme - version perhaps being "dont use getters in your classes, period." The reasons can often be on a quite abstract level - "getters and setters promote coupling" (what's coupling again?), "an interface must only define behavior and not state" (eh… why's that?), "getters breaks encapsulation" (encapsuh… muh?) One can refer to some code-smell we haven't read about - like that getters are an indication of feature-envy and then top it off with simplistic catch-phrases like "tell, don't ask" - a shorthand for the Law of Demeter for functions (Law? We have laws now? I just want to program!). Mockery aside, I of course agree with all of them. Writing getter-less code is often a very sensible strategy - in the same sense that breathing is often a very sensible strategy, even though there are times you actually would like to hold your breath1. And in my opinion the worst kind of getter, is an abstract getter - or a getter in a interface.

I really like the concept of interfaces. 2 Inserted at the right places they can, as a hot knife through butter, decouple and/or encapsulate large pieces of otherwise strongly coupled unmaintainable legacy code. Used well, interfaces promote the elusive, valuable and sought-after property we might call "testability" - the property code has when it is easy to write a (unit-) test for new functionality that also utilize existing parts of the code (e.g. the usual stuff one does while developing applications and libraries.)3 Used wrongly, interfaces kill enough "testability" to ruin my working-day in a second. And take "ruin my working-day" to mean "incurring unnecessary extra cost on the project."

Lets just get a couple of things off the table right away. First of all, I'll just assert without further argument that testability is a property that we both want and need, and that it is cost efficient to ensure that we have it. Second, testability is brittle - there are probably as many ways to break it as there exists software developers. Third, in order to be testable our code must be SOLID. In other words - testability is something that don't come naturally, and it is something that we, as software developers, need to be a little bit obsessed about. I've usually worked with C++, and I would prefer to do my job doing test-driven development 4. In order to be efficient in doing so, the codebase must be testable. The goal with this post is just to elaborate a little bit on how getters in interfaces totally kills testability (and my TDD-zen.)

Consider the following artificial source, purpose built to demonstrate what happens when interfaces define getters. The system-under-test (SUT) is the class NewFeature. This is the object that has behavior I need to test. The method I need to test is of course behaviorToTest, which needs an instance of type Foo as an argument. Foo is an interface, and Foo exposes some other types and interfaces through getters (put there to exemplify different situations arising from getters in interfaces.) Foo, Bar and Baz are written and maintained by "someone else".

class Foo;

class NewFeature {
public:
  NewFeature();
  void behaviourToTest(Foo & the_Foo);
};


class Bar;
class Baz;

typedef std::shared_ptr<Bar> BarPtr_t;
typedef std::shared_ptr<Baz> BazPtr_t;
typedef std::vector<BazPtr> BazPtrVector_t;

class Foo {
public:
  virtual ~Foo(){}
  virtual size_t getCurrentNumber() const = 0;
  virtual BarPtr_t createBar() const = 0;
  virtual const BazPtrVector_t & getBazVector() const = 0;
  virtual void nonConstBehaviour() = 0;
};

class Bar {
public:
  virtual ~Bar(){}
  virtual void keepBarIng() = 0;
};


class Baz {
public:
  virtual ~Baz() {}
  virtual void identifyYourBearings() = 0;
};

Let's just agree that as far as we can see, this code is SOLID. Or, okay - whether it follows the single responsibility principle or not is hard to tell, as they are all just junk-names (and if I squint I'd might say that there are indications that implementations of Foo is supposed to do a lot more than one thing.) We don't know much about Liskov's substitution principle either, as being compliant is in many ways up to those implementing these interfaces - but lets assume all implementations of Foo are compliant. Open-Closed, Dependency Inversion and Interface segregation looks all catered for.

So… how does this SOLID code manage to break my TDD-zen? Lets begin with writing a test testing one aspect of the SUT…

void
TestNewFeature::test_thatTheFeatureBehaves()
{
  NewFeature sut;
  sut.behaviourToTest(
  // Doh. I need a concrete Foo. And thus I need a concrete Bar. And I'm
  // also likely to need a bunch of concrete Baz-es, in a vector of shared
  // pointers that is. Who the h#%% wrote this $#|+?
}

And behold - before long, I need to roll three test-doubles (or mocks) of my interfaces, and the stuff returned by their getters must make sense to my code. Further, the getters are giving quite strong requirements on how these stubs or mocks must be implemented.

The first method, Foo::getCurrentNumber(), is probably the easiest to implement. It seems like it reports about some internal state of Foo, and that it will have some effect on the behavior we are implementing in NewFeature. So at least, the number returned must be meaningful (the application context will provide the context for figuring what is 'meaningful') It is also likely that such a getter in a real interface force us to implement and keep track of some meaningful state within the stub - complicating the implementation of the test-code (increasing the possibility that the stub it self contains a bug.) All in all, in addition to increase the time spent writing the test, this getter makes the test harder to maintain as "more code must be right" for the test(s) to be right.

Foo::createBar() is obviously a factory method. Hence, it can be allowed to return a new instance of Bar every time. That is actually quite okay - "create" is a behavior good as any. But - still I do need to find or write a concrete Bar-class, and this created object perhaps need to track the number of calls to keepBarIng() in order to verify the behavior of NewFeature. The work needed to perform just to write this one simple test is increasing, and the likelihood that Average Joe Developer actually will write it is shrinking dramatically with this added amount of work and hazzle.

Then, the last getter is Foo::getBaz(). This is the worst kind of getter. This is the kind of getter that makes me go to Hell for use and abuse of profanities. Because it returns not only a vector of pointers, but a const reference to a vector of pointers. In other words, in order to get my stub up and running, it must have a field to hold that vector of Baz-pointers. Actually, all classes that inherits from Foo requires a field to hold that vector of Baz-pointers… (The getter breaks encapsulation, because it reveals details of the implementation, and it actually enforce duplication across implementations.) Not to mention the work I need to perform to roll meaningful concrete Baz types and populating the test-vector with them. The likelihood that this test ever gets written, is plummeting. Like a sack of tomatoes dropped out of a low-flying aeroplane as a step towards making the worlds biggest greek salad 5.

Finally for the worst part. NewFeature::behaviorToTest should function correctly no matter what version of Foo it get. And it is not in the place of NewFeature to define what the correct behavior of Foo and friends is - like for example asserting or throwing exceptions if Foo does something NewFeature don't expect. This is a touch of the Liskov Substitution Principle: Foo's behavior is defined by the already more or less established contract for correct Foo-behavior, and NewFeature must roll with that - the only 'change' to the established contract of correct Foo-ing NewFeature can perform, is accepting a wider range of behaviors. And - what can happen behind the scenes in implementations of the Foo interface? Take the methods Foo::nonConstBehavior() and Baz::identifyYourBearings() - NewFeature can use them (and they might be the main reason for being passed Foo in the first place), and through internal wiring in Foo, calling those methods might end up changing the internal state of the Foo-implementation. As this state leaks out via the getters, NewFeature must (surprisingly) anticipate such changes. Disaster ensues! Both methods can for example potentially change what Foo::getCurrentNumber() and Foo::getBazVector() would return. Even worse, so can Bar::keepBarIng() as far as we can tell. And even though Foo::createBar() looks like a regular factory method, it would come as a unwelcome surprise if the created object actually can change the internal state of the Foo object. All these side-effects are stupid6, but possible7. How the classes are allowed to interact in a 'correct' implementation of Foo when Foo have getters, is at best muddy. Some of these cases are likely to occur and then they actually need to be tested (if we want to be certain NewFeature behaves correctly) - adding to the number of tests that need to be written. All of a sudden, testing NewFeature::behaviorToTest have turned in to an ambiguous time-consuming nightmare.

And this is then the point of this blogpost: While interfaces in C++ is a honking great idea and in general promotes testability, getters in interfaces require clients to write more scaffolding, more test-code and more tests in the face of ambiguous behavior (caused by the getters!) This makes it harder to write tests, thus reducing testability. It makes it more likely that code using these interfaces are not covered by unittests.

So - TDD-Zen broken and all… What to do, if I end up in some situation like this? With a handful of getter-ridden coupled interfaces provided and maintained by others? Awh… then I might just think "craps", scrap my test, and cram my stuff in to the NewFeature and go home to a sleepless night contemplating the havoc that might be caused by NewFeature not being tested in a proper fashion. I also might of course actually write the stubs and mocks I need, or find some that someone else wrote, and then test NewFeature. But I still would be annoyed enough with both the unnecessary cost incurred while implementing the (scaffolding needed for the) test and the problems (e.g. future costs) that will arise from maintenance of the SUT when I'm long gone from the project, that I would write a blogpost about it.

Footnotes:

1 So there's no need to tell me that you need a getter from time to time. I know.

2 Java and C# come with interfaces defined as part of the language, in C++ this means classes with only pure virtual methods.

3 My "definition" of testability implies code that have low coupling.

4 Or Test Driven Design if you so prefer

5 Did I talk about the likelihood, or the plummeting?

6 Stupid, in software development jargon, means unmaintanable, bug-ridden and unnecessarily complex

7 Possible, in software development jargon, means "it occurs somewhere in your current codebase, and YOU made it happen."