Wednesday, 18 September 2013

Virtual Methods in C# Are a Design Smell

I came to C# after having spent 15 years writing applications and services in C++. In C++ there is no first class concept for “interfaces” in the same way that C# and Java has. Consequently they were emulated by creating a class that only contained pure virtual functions:-

class IConnection
{
  virtual void Open() = 0;
  virtual void Send(Message message) = 0;
  virtual void Close() = 0;
};

C# != C++

In C# however the interface is a first class concept and naturally when I started using C# I automatically made the implementation of my interface methods virtual [1]:-

interface IConnection
{
  void Open();
  void Send(Message message);
  void Close();
}

public class Connection : IConnection
{
  public virtual void Open()
  { }
  public virtual void Send(Message message)
  { }
  public virtual void Close()
  { }
}

Of course I very quickly discovered though that interfaces in C# do not rely on v-tables in the same was as C++ and that an interface method is altogether another concept over and above what virtual methods give you. Throw in Explicit Interface Implementation and you can seriously mess with the Liskov Substitution Principle (the L in SOLID) [2].

Once I understood that interface methods didn’t need to be virtual I also realised that most of the classes I create, and have been creating, are not actually polymorphic. In C++ they tend to look that way because it’s the only mechanism you have, whereas in C# it seems much clearer because of the more formal distinction between implementation inheritance and just implementing an interface. Putting aside my initial fumblings with C# I think I can safely count on one hand (maybe two) the number of times I’ve declared a method “virtual” in C#.

Testability

A common reason to mark a method as virtual is so you can override it for testing. The last time I remember creating a virtual method was for this exact purpose. I had a class I wanted to unit test but it ran “tasks” on the CLR thread pool which made it non-deterministic and so I factored out the code that scheduled the tasks into a separate protected virtual method:-

public class Dispatcher
{
  . . .
  public void DispatchTasks()
  {
    . . .
    foreach (var task in tasks)
        Execute(task);
    . . .
  }

  protected virtual void Execute(MyTask task)
  {
    ThreadPool.QueueUserWorkItem((o) =>
    {
      task.Execute();
    });
  }
  . . .
}

This allowed me to derive a test class and just override the Execute() method to make the asynchronous call synchronous by using the same thread:-

public class TestDispatcher : Dispatcher
{
  protected override void Execute(MyTask task)
  {
    task.Execute();
  }
}

Favour Composition Over Inheritance

Looking back, what I probably missed was a need to separate some concerns. The dispatcher class was responsible for not only finding the work to do, it was also responsible for queuing and scheduling it too. My need to mock out the latter concern to test the former should have been a smell I picked up on. In many cases where I’ve seen virtual methods used since it’s been to mock out a call to an external dependency, such as a remote service. This can just as easily be tackled by factoring out the remote call into a separate class with a suitable interface, which is then used to decouple the caller and callee. This is how you might apply that refactoring to my earlier example:-

public interface IScheduler
{
  void Execute(MyTask task);
}

public class ThreadPoolScheduler : IScheduler
{
  public void Execute(MyTask task)
  {
    ThreadPool.QueueUserWorkItem((o) =>
    {
      task.Execute();
    });
  }
}

public class Dispatcher
{
  public Dispatcher()
    : this(new ThreadPoolScheduler())
  {  }

  public Dispatcher(IScheduler scheduler)
  {
    _scheduler = scheduler;
  }

  public void DispatchTasks()
  {
    . . .
    foreach (var task in tasks)
        _scheduler.Execute(task);
    . . .
  }
  . . .
  private IScheduler _scheduler;
}

I could have forced the client of my Dispatcher class to provide me with a scheduler instance, but most of the time they would only be providing me with what I chose as the default anyway (there is only 1 implementation) so all I’m doing is making it harder to use. Internally the class only relies on the interface, not the concrete type, and so if the default eventually becomes unacceptable I can remove the default ctor and lean on the compiler to fix things up.

Too Much Abstraction?

One argument against this kind of refactoring is that it falls foul of Too Much Abstraction. In this example I believe it adds significant value - making the class testable - and it adds little burden on the client too. In general I’ve worked on codebases where there is too little abstraction rather than too much. Where I have seen excessive layers of abstraction occur it’s been down to Big Design Up-Front because code that is built piecemeal usually has abstractions created out of a practical need rather than idle speculation.

 

[1] Personally I blame COM which had #define interface struct so that you could use the “interface keyword” in your C++ code.

[2] Thanks goes to Ian Shimmings for showing me where using Explicit Interface Implementation goes against LSP for what looks like an acceptable reason - Fluent Interfaces.

Tuesday, 17 September 2013

Feature Branch or Feature Toggle?

One of the great things about joining a new team is having the opportunity to re-evaluate your practices in light of the way other people work. You also have a chance to hear new arguments about why others do things differently to the way you do. One recent discussion came about after I spotted that a colleague pretty much always used a branch for each (non-trivial) feature, whereas I always tend to use main/trunk/master [1] by default.

Martin Fowler wrote about both Feature Branches and  Feature Toggles a few years ago and ever since I worked on a project where there were more integration branches than a banyan tree [2] I’ve favoured the latter approach. Working with a source control system that doesn’t support branching (or does but very poorly, like SourceSafe) is another way to hone your skills at delivering code directly to the trunk, without screwing up the build (or your teammates).

Branching

The reason for branching at all is usually because you want some stability in the codebase whilst you’re making changes. Your view/working copy [3] is clearly isolated from other developer’s but unless you have the ability to commit your changes in stages it’s not exactly the most productive way to work. The most common non-development branch is probably the Release Branch where changes are made very carefully to maintain stability up to deployment and then to act as a safe place to create fixes and patches to address any serious problems.

I find myself very rarely wanting to branch these days. It’s more likely that I decide to shelve [4] my changes to reuse my working copy for something else, e.g. fix a build/deployment problem, after which I’ll un-shelve and carry on as if nothing happened. I don’t work in the kind of environments where spikes are very frequent; if anything it’s a potentially messy refactoring that is likely to cause me to branch, especially if the VCS doesn’t handle moving/renaming files and folders very well, like Subversion. Using the Repo Browser directly in Subversion with a private branch is probably the easiest way to remain sane whilst moving/renaming large folders as it saves on all the unnecessary shuffling of physical files around the working copy.

Toggling

My preference for publishing changes directly to the integration branch is borne out of always wanting to work with the latest code and keeping merges to a minimum. That probably sounds like I’m being a little hypocritical after what I said in “What’s the Check-In Frequency, Kenneth?”, but I try and keep my commits (and/or pushes) to a level where each change-set adds something “significant”. This also allows me to fix and commit orthogonal issues, like build script stuff immediately without having to cherry-pick the changes in some way.

Generally speaking, new code tends to have a natural “toggle” anyway, such as a menu entry or command line verb/switch that you can disable to hide access to the feature until its ready. When the changes go a little deeper I might have to invent a switch or .config setting to initially enable access to it. This way the feature can be tested side-by-side with the existing implementation and then once the switchover has occurred the default behaviour can be changed and the enabling mechanism removed. The need to do this kind of thing comes out of the way some organisations work - a lack of formal acceptance testing means the change practically reaches production before it’s known whether it will go live or not!

Waste Until Proven Useful?

What I found interesting in our discussion of the branch vs toggle approach was that my colleague felt he needed to keep his “work-in-progress” code out of the integration branch until it was mature. Looking at it from a lean perspective I guess you could argue that a feature that is not enabled is just waste and so shouldn’t exist - it’s dead code. This kind of make sense, but I think it turns a feature into an all-or-nothing proposition and there may be value alone in any refactoring that has been done to get the feature implemented. I would want that refactoring to take effect in the development integration branch as soon as possible to give it time to bed in.

I guess what makes me comfortable with working directly on the trunk is having 20 years experience to draw on :-). I have the ability to break down my changes into small tasks, and I know what kinds of changes are highly unlikely to have any adverse effects. This also presupposes the codebase is being developed by other people that also don’t have a habit of doing “weird stuff” that is likely to just break when seemingly unrelated changes occurs. All the usual good stuff like decent test coverage, low coupling and high cohesion makes this style of working achievable.

 

[1] Have we reached a general consensus yet on what we call the major “development” integration branch? I know it as “main” from ClearCase, “trunk” from Subversion and “master” from Git. SourceSafe doesn’t have a name for it because it doesn’t exactly make branching easy.

[2] Not sure if they have the most branches, but they sure seem to have a lot!

[3] Once again, I have “view” from ClearCase and “working copy/folder” from Subversion/SourceSafe.

[4] And yet another term - “shelve” or “stash”?

Monday, 16 September 2013

Overcoming the Relational Mindset

My current project is a bit of a departure for me as I’ve left behind the world of the classic SQL RDBMS for a moment and am working on one of those new-fangled NOSQL alternatives - MongoDB. Whilst I haven’t found any real difficulty adjusting to the document-centric world (thanks to too much XML) I have noticed myself slipping back into the relational mindset when making smaller changes to the schema. One such example happened just the other day…

Stop Extending Tables

Imagine you’re working for a retailer that has some form of loyalty card mechanism. Whenever you make a purchase you are told the number of points you have received for the current purchase, plus any accrued up to some date (notionally today). The initial part of the document schema might look something like this:-

LoyaltyBonus:
{ 
  CardNumber: “1234 5678”, 
  Points: 100
}

Now, the second part - the accrued points to date - has a slight twist in that the service required to obtain this data might not be available and so it’s not always possible to obtain it. Hence that part of the structure is optional. Slipping back into the relational mindset I automatically added 2 nullable attributes because what I saw was a need to extend the LoyaltyBonus “table” with two optional values like so:-

LoyaltyBonus
{ 
  CardNumber: “1234 5678”, 
  Points: 100, 
  BalancePoints: 999,        // Optional 
  BalanceDate: “2013-01-01”  // Optional
}

…and when the loyalty service is not available it might look like this:-

LoyaltyBonus:
{ 
  CardNumber: “1234 5678”, 
  Points: 100, 
  BalancePoints: null, 
  BalanceDate: null
}

Of course the null values can be elided in the actual BSON document but I’m showing them for example’s sake. The two attributes BalancePoints and BalanceDate are also tightly coupled, either they both exist or neither does. That might seem fairly obvious in this case, but it’s not always.

Documents, Not Columns

What I realised a little while later (after peer reviewing someone else’s changes!) was that I probably should have created a nested document for the two Balance related fields instead:-

LoyaltyBonus:
{ 
  CardNumber: “1234 5678”, 
  Points: 100, 
  Balance: 
  {  
    Points: 999, 
    Date: “2013-01-01” 
  }
}

Now the Balance sub-document exists in its entirety or not at all. Also the two values are essentially non-nullable because that’s handled at the sub-document level instead. The other clue, which in retrospect seemed blindingly obvious [1] was the use of the prefix “Balance” in the two attribute names.

 

[1] It’s never quite that simple in practice because you have probably already gone through a number of other refactorings before you got to this point. In a sense it’s a bit like going through the various Normal Forms in a relational schema - at each step you need to re-evaluate the schema and pull out any more sub-documents until you’ve factored out all the optional parts.

Friday, 6 September 2013

OwnedPtr and AssocPtr - UML in C++

My recent post about “Overdoing the References”, coupled with another recent post from an ex-colleague (Carl Gibbs) titled “Divide in C++ Resource Management” caused me to remember an idea we tossed about around the turn of the millennium for representing UML ownership semantics in C++...

Back then general purpose smart pointers, and in particular reference-counted smart pointers were still fairly cutting edge as Boost was in its infancy (if you were even aware of its existence). Around the same time UML was also gaining traction which I personally latched onto as I found the visualisation of OO class hierarchies jolly useful [1]. What I found hard though was translating the Aggregation and Association relationships from UML into C++ when holding objects by reference. This was because a bald (raw) pointer or reference conveys nothing about its ownership semantics by default. References at least had the convention that you don’t tend to delete through them (if you exclude my earlier reference obsessed phase), but that wasn’t true for pointers.

Unique Ownership

Reference-counted smart pointers like std::shared_ptr<> are the Swiss-Army knife of modern C++. The common advice of not using std::auto_ptr<> with containers is probably what led to their adoption for managing memory everywhere - irrespective of whether the ownership was actually shared or logically owned by a single container, such as std::vector<>. My overly literal side didn’t like this “abuse” - I wanted ownership to be conveyed more obviously. Also, the number of places where shared ownership even occurred was very rare then because there was always an acyclic graph of objects all the way down from the root “app” object that meant lifetimes were deterministic.

UML in C++

image

The canonical example in UML of a where both forms of ownership crops up is probably with a tree structure, such as with the nodes in an XML document. A node is a parent to zero or more children and the relationship is commonly bidirectional too. A node owns its children such that if you delete a node all its children, grand-children, etc. get deleted too.

Using bald pointers you might choose to represent this class like so:-

template<typename T>
class Node
{
private: 
  T                  m_value; 
  Node*              m_parent; 
  std::vector<Node*> m_children;
};

However you could argue there is a difference in ownership semantics between the two Node* based members (m_parent and m_children). The child nodes are owned by the std::vector<> container, whereas the parent node pointer is just a back reference. Naive use of reference-counted smart pointers for both relationships can lead to memory leaks caused by the cyclic reference between parent and child and so by keeping the child => parent side of the link simple we avoid this.

The Smarter Pointer

So, to deal with the ownership of the children we came up with a std::auto_ptr<> like type called OwnedPtr<>. The idea was that it would behave much like what we now have in the std::unique_ptr<> type, i.e. std::auto_ptr<> like non-shared ownership, but without the container problems inherit with auto_ptr<>.

  Node*                       m_parent; 
  std::vector<OwnedPtr<Node>> m_children;

The Dumbest Pointer

Whilst we could have left the child => parent pointer bald, this meant that it would be hard to tell whether we were looking at legacy code that had yet to be analysed, new code that mistakenly didn’t adhere to the new idiom, or code that was analysed and correct. The solution we came up with was called AssocPtr<> which was nothing more than a trivial wrapper around a bald pointer! Whilst it was functionally identical to a raw pointer, the name told you that the pointer was not owned by the holder.

  AssocPtr<Node>              m_parent; 
  std::vector<OwnedPtr<Node>> m_children;

Exit UML / Enter shared_ptr

In the end this idea became just another failed experiment. The OwnedPtr<> type was pretty indistinguishable from a classic reference-counted smart pointer and ultimately it was easier to just share ownership by default rather than decide who was the ultimate owner and who was just a lurker. Once Boost showed up with its various (thread-safe) smart pointer classes the need to crank one’s own variants pretty much evaporated.

[1] I also thought the Use Case, Deployment and Sequence diagrams were neat too. While I still find value in the latter two I got disillusioned with the Use Case aspect pretty quickly.

Thursday, 18 July 2013

Redirecting Output to Dated Log Files

The other day the requirement came up to run a scheduled task to copy some data from a production to development machine to archive some data for testing later. Naturally ROBOCOPY was going to do the heavy lifting, but if there’s one thing I’ve learnt about scheduled tasks it’s that you’ll always want to capture the output to a log file, if it’s not done already by the tool, and to annotate that output with extra details like the start/end date & time.

Redirecting Output

Taking a leaf out of my own book, or more specifically my recent “In The Toolbox” article on wrapper scripts in the current C Vu, I’d probably wrap the command in a batch file to give myself a little wriggle room and then capture the entire output by relying on the command interpreter to do the redirection. For example the task definition might look like this:-

C:\Program Files\My Service\ProdToDevCopy.cmd >> D:\My Service\Log\ProdToDevCopy.log 2>&1

As a general rule I always append to log files rather than clobber them, so that you maintain a history of failed runs when they occur. The “2>&1” bit on the end [1] redirects the stderr stream to stdout so that it goes in the same log file. I find two log files a bit of a pain, especially when you’re trying to track down why it failed. Ands I often forget to add this, which is another reason why I’m writing this down now.

Dated Log Files

When a task runs very frequently using a single log file gets unwieldy, not least because it makes archiving old log files impossible. Consequently logging to a file based on the current date is usually a sufficient way to split them. Another reason to implement the scheduled task command as a batch file is that what you’re really invoking is a copy of the command interpreter (as CMD.EXE /C)and so you have access to other features too, such as the lesser known “substring” expansion feature for variables. If you type C:\> HELP SET you’ll find out all the details.

In this instance we want to take the output of the %DATE% variable, split it into its separate fields and then recombine them again to make an ISO style date (YYYY-MM-DD) [2]. On my machine the output for %DATE% is in the common DD/MM/YYYY format:-

C:\> echo %DATE%
18/07/2013

Before going any further it should be apparent that this is not going to work on an arbitrary machine because different locales mean that the source date format will vary. However in the context of an enterprise application server where a lot of things can often be taken for granted this can work just fine.

The basic format for a variable expansion is this:-

%<name>:~<offset>,<length>%

The offset and length can both be negative meaning that you can index from the end of the string and take all but a few characters. And you can omit them too which implies an index of 0 and the entire string length.

Just using the indexing gives you at least two usefully different ways to represent the window:-

YYYY = %DATE:~6,4% or %DATE:~-4%
MM   = %DATE:~3,2% or %DATE:~-7,2%
DD   = %DATE:~,2%  or %DATE:~-10,2%

And so applying the shortest form of each we can concoct a minimal pattern for our date:-

C:\> echo %DATE:~-4%-%DATE:~3,2%-%DATE:~,2%
2013-07-18

Plugging this back into the (shortened) original script command line gives us this:-

ProdToDevCopy.cmd >> D:\Log\ProdToDevCopy.%DATE:~-4%-%DATE:~3,2%-%DATE:~,2%.log 2>&1

 

[1] And it has to go on the end too. If you put it before the redirection it won’t work. For an extensive run-down of redirection options see this page.

[2] By using an ISO style date in log file names they become sorted by date by default which can make life easier in some scenarios.

Tuesday, 16 July 2013

Overdoing the References

Thinking and reading about handling nulls again over the last few days has had an interesting side-effect of bringing some clarity to a phase of my C++ learning that I always thought was somewhat weird; I call it my “everything must be a reference” phase. Basically I stopped using pointers in my C++ code and started using references instead, everywhere. Naturally this lead to some very odd looking code that you can still see in my legacy C++ libraries to this day.

Reference Free Beginnings

Like many programmers I started out writing in C, where the only choice you had was the pointer. A consequence of its duality (null or value) meant that just by looking at the interface to a function you couldn’t really tell if the parameters were optional or not, if they weren’t of the primitive (built-in) types.

struct Person
{
    const char * m_name;
};

void printPerson(const Person* person)
{
  . . .
}

Of course you had a fairly good idea that in most cases the value wasn’t optional and so books like Steve Maguire’s “Writing Solid Code” introduced me to the notion of Design by Contract and using ASSERTs in your interfaces to spot and highlight early when the contract had been violated [1]. The knock-on effect of that, at least for those like myself who might have taken some of the advice a little too literally, was that our codebase became utterly littered with ASSERTs.

void copyPerson(Person* lhs, const Person* rhs)
{
  ASSERT(lhs != NULL);
  ASSERT(rhs != NULL);
  . . .
}

In order to catch the violation as early as possible you needed to apply these checks as often as possible. At least, that was the logic I ended up applying. And to the extent that I started using ASSERTs where I probably should have been validating inputs, but that’s another post for another day...

From Pointers to References

Moving from mostly doing C to mostly doing C++ meant that I entered a brave new world with the notion of “references”. As I understood it back then references were basically the same as pointers (in implementation) but they were mostly an enabler for things like overloading the assignment operator and allowing you to use “.” instead of “->” [2]. As someone coming from C that sure sounded like a bit of a kludge to me.

Although I embraced references to a certain degree, such as where they were expected, like in a copy constructor and assignment operator, I really didn’t get them at first. Some things just didn’t add up. For one thing why was “this” a pointer and not a reference? Then there was this “folklore” around the fact that references can’t be NULL. Yes they can, I can just do this:-

Person& person = (*(Person*)NULL);

OK, so that’s a bit contrived, but I was still hitting bugs where references were null, just as much as if they were declared as pointers, so clearly they weren’t any better from the perspective of avoiding dereferencing NULL pointers.

In search of an answer about the whole “this as a pointer” and other questions I picked up a copy of The Design and Evolution of C++. This is a great book which any self-respecting C++ programmer should read, not only because it gives you some insight and context into the history of various language features, but it also shows you the ever growing set of constraints that are applied to every subsequent change in the language.

Anyway, I got some answers from this and more importantly I began to realise that the “nullness” of a reference was less about the physical side and more about the semantics - using references meant that logically the parameter could never be null. A side effect of that is that no ASSERT is required in the function prologue because the interface already declares its intent. Doh!

References Everywhere

That got me thinking. If I use references everywhere and turn any pointer into a reference as soon as possible then I can avoid nearly all this unnecessary pointer checking repetition. Being the ridiculously literal person I am at times I even took that to things like the return from the new operator by introducing simple factory methods like this:-

Row& Table::CreateRow()
{
  return *(new Row(*this));
}

If you’re thinking to yourself that’s an exception unsafe disaster waiting to happen, let me show you the call site:-

Row& row = m_clocks.CreateRow(); 

try 
{ 
  row[Clocks::COMPUTER] = m_machine; 
  . . . 
  m_clocks.InsertRow(row); 
} 
catch (…) 
{ 
   m_clocks.DeleteRow(row); 
}

Only kidding, it’s still a maintenance headache. At the time I wrote this Boost was by no means as ubiquitous as it is today and we were even a few years off C++ 03 so shared_ptr<> was a still a bit “new fangled” and experimental [3]. And that assumes Visual C++ would get its act together and we would be willing to entertain the idea of living on the bleeding edge. Nope, neither as going to happen.

Ownership

Although I could see the utility in shared_ptr<> and I was just beginning to get my head around the more general notion of RAII and writing exception-safe code, it still felt a bit like a backwards step. The desire to reduce the complexity of client code by leaning on RAII more meant the switch back to using what is fundamentally a shared “pointer”, not a shared “reference”. I know I’m not the only one who found this notion confusing because I’ve come across some weird home grown smart pointer classes in my time on other codebases.

So, now we have three choices in our API: pointers, references and the new one - smart pointer by reference. Due to some fairly dodgy custom smart pointer implementations that had a very noticeable effect on performance [4] I started seeing functions that took a smart pointer by reference instead of by value:-

void DoStuff(const shared_ptr<Thing>& thingy);

But isn’t this really all about confusing ownership semantics, not optionality? If you take ownership out of the equation for a moment the callee can expect a bald pointer or a reference. However if the callee wants to take shared ownership of an object and it won’t take NULL for an answer how should it declare its intent?

I’m still chewing over what the man himself (Bjarne Stroustrup) said at this year’s ACCU conference, but if I’ve understood him then I should only ever be passing values around anyway - any ownership and performance problems from copying should just be an implementation detail of the type. And therefore I shouldn’t be passing around shared_ptr<T> style values, but the T’s themselves (presumably using the Handle/Body idiom) [5].

Closure

I’m glad it’s only taken me just over a decade to finally work out what on earth I was trying to do all those years ago. I’m quite sure somewhere there is some code in production that a poor maintenance programmer is scratching their head over right now whilst the WTF meter is going through the roof. If it is you, then I can only apologise.

 

[1] In this era and the applications I worked on you couldn’t afford to keep this code in your release build so it only helped you during development, but it did make a big difference in tracking down bugs at the time.

[2] Much as I agree with the likes of Michael Feathers and Stan Lippman about knowing what’s going on under the hood helps you make more informed choices about using the language efficiently, it could also be a double-edged sword if you’re perhaps not aware of the concepts being modelled.

[3] I suspect if I had moved in the right circles, or had been a member of the ACCU back then I might have been able to spare my blushes a little.

[4] On Windows using a mutex is a very heavy-handed way of making a smart pointer thread safe. Critical sections were often employed too despite the primitive InterlockedXxx functions being readily available. Mind you I’ve no idea what the state-of-the-art is in thread-safe smart pointer implementations these days. Perhaps they’re consigned to history like the Reference Counted String class?

[5] At some point I’ll go back and watch the ACCU keynote video, not least to snigger at his poke at Java, but also to try and take in what he said again. At the time I didn’t think he said that much, and maybe he didn’t, it’s just that now maybe I’ve started to actually listen...

Tuesday, 9 July 2013

Null Checks vs Null Objects

In my last post “The Application-Wide Diagnostic Log - the One Acceptable Global Object?” I had the following sort of code in my logging facade:-

  if (ApplicationLog != null)
    ApplicationLog.Error(format, args);

The reason for the check against null was because my first requirement of the facade was that it should be no burden at all when testing. I don’t want to have to create some sort of mock every time I write a test for some code that happens to have logging statements in it. I thought I had the whole issue covered too when I added the following comment:-

“I can easily create a Null Object implementation, although with the checks that’s fairly pointless unless you want to avoid them.”

Given the subject matter, which I knew would be fairly contentious, I was half expecting a tirade against my suggestion of even allowing one global variable. What I didn’t expect was something that would highlight a rather different hole in my understanding of the Null Object pattern. It arrived courtesy of fellow ACCU member Alan Stokes via this tweet:-

“Surprised you prefer the null checks to Null Object - they're pure waste in the common case. Also null is pure evil.”

The part of that tweet I like most is “you prefer the null checks”. It’s so polite in suggesting that my skill at programming is such that I consciously made the decision to implement my facade using a null check rather than using the Null Object pattern. Of course what we all really know is that I made the unconscious decision and just went with what I “instinctively” thought was right. Which, after even a moments thought, I realised it clearly wasn’t [1].

Null Objects as a Convenience

Alan is right in that null references make code more complicated than necessary, and it has always been this facet of the Null Object pattern that I’ve focused on. As a general rule I try and avoid passing null references around, especially to other services. If I need a constructor where one parameter is optional I’ll implement it as two separate constructors and apply a “not null” constraint to ensure the caller has to play ball:-

public class Service
{
  public Service()
    : this(new OtherServiceNullImpl())
  {
  }

  public Service(IOtherService otherService)
  {
    Constraint.MustNotBeNull(otherService);
    . . .
    // Other initialisation code
  }
}

Here I want to enforce the constraint on the public interface of Service that I’m not tolerating null references. I have the choice internally to either delegate both constructors calls to a single internal helper method that does allow nulls, or chain the constructors and pass a null implementation when not provided by the caller. The former choice means I’ll probably still have to add null checks internally in my class, but at least it’s inside my own four walls - my implementation choice does not affect my clients. If I’m happy to waste time creating a potentially unused object I can simplify things even further by just defaulting to the null implementation and then replacing it if the more specific ctor is called:-

  private IOtherService m_other = new  
                        OtherServiceNullImpl();

The same idea applies when you are returning a reference. Imagine the following method on the service:-

public List<Customer> FetchCustomers()
{
    var customers = new List<Customer>();
    . . .
    return customers;
}

When there is nothing to return, you could return a null reference, or you could return an empty collection. I would hope most developers see that returning the null reference is a real pain as it hinders any sort of composition, such as with the LINQ extensions. But what about string values though? I went through that thought exercise a couple of years ago when I first started doing C# and filed it under “Null String Reference vs Empty String Value” [2].

Switching back then to the logging example in my previous post the case for use of a null object begins to look much clearer. If we allow the ApplicationLog property to be set to null we make it much harder for any consumers to use - it’s essentially the same as the return value case above. So, perhaps I should have promoted a different design which defaults to a null logger instead and disallows anyone to set the global logger to a null reference:-

public static class Log
{
  // The application-wide log.
  public ILogger ApplicationLog
  {
    get
    {
      return s_applicationLog;
    }
    set
    {
      Constraint.MustNotBeNull(value);
      s_applicationLog = value;
    }
  }

  // Write an error message
  public void Error(string format, params object[]
                                              args)
  { 
    s_applicationLog.Error(format, args);
  }

  private static ApplicationLog s_applicationLog =
                                new NullLogger();
}

Now we no longer need any null checks within our code or in any callers that should decide to partially take matters into their own hands, but still want to write through the global logger.

Null Objects as an Optimisation

The hardest thing about being an experienced developer is that you have to remember to unlearn stuff as the world around you changes. Over time you slowly build up knowledge about what works and what doesn’t and you generally get a feel for what things cost. Of course we all know that nothing should be taken at face value and that we should measure everything before discounting it. But that’s really what experience is for isn’t it - to short-circuit the blind alleys? Except of course when it’s not…

I put forward TDD as my defence [3] for not spotting what is pretty clearly a case of not optimising for the common case. My first requirement for the logging facade was the following:-

“It must, by default have no impact on unit testing. I should not need to do anything to write a test for existing code or be able to add logging to existing code without it breaking the build ”

As such I wrote all the tests and designed the facade entirely based on the premise that I wanted the call to log a message to be a NOP (or as close to one as possible) by default. Now, putting aside for the moment the fact that modern CPUs handle branch predication and v-tables style dispatch more efficiently than they used to [4] and that JIT compilers are also pretty smart, this still misses the simple fact that the common case is that an instance of logger object will exist. That’s right, every service and component I write will have (some form of) logging enabled by default so the “else” branch will never get taken in a production scenario, doh!

Perhaps it’s also worth looking at the problem another way. For the cost of the difference between using an “if” statement and calling a null object to matter you would need to be willing to accept the cost of writing to the log sink (probably a file) which would undoubtedly be orders of magnitude greater. Even if you’re going to buffer the writes and do it on another thread it’s still going to cost you fair bit in comparison.

There. I’ve just done it again, haven’t I? After spouting on about the need to measure and not relying on old rules of thumb I’ve just surmised the cost is insignificant. And they say you can’t teach an old dog new tricks...

[1] This is what I enjoy most about blogging - it’s like a public design review. As long as there is someone out there (hopefully more knowledgeable than me) willing to stick their neck out too and question what I’ve written, I’ll get to see my mistakes and shortcomings and hopefully learn something.

[2] Interestingly that blog post of mine even has a section titled “Null References Are Evil”, but I see that once again I’m only focusing on the convenience aspect of null objects.

[3] Yeah, because I clearly can’t just admit that I was being lazy now, could I…

[4] I’ve worked on a couple of applications in the (very dim and distant) past where the cost of the virtual function call machinery was a significant overhead. Consequently the thought of a dynamically dispatched NOP call immediately “feels” excessively costly. Note to self: Google some modern benchmarks to see how insignificant it probably is today.