Mock To Test the Outcome, Not the Implementation

The other day a colleague came to me with a question about a problem they had trying to verify an expectation through a mock – they were trying to mock the invocation of an indexer property in C# with Rhino Mocks. Not knowing Rhino Mocks at all well I had to pass on the specific question, but it did raise other questions and it seems there was a ‘smell’ emanating from the test & code that my colleague had also picked up on…

The Feature

The code they were trying to test was a factory method that was going to be invoked by some 3rd party application framework. The method it seems has more than one responsibility forced upon it by the framework. The code  looked something like this:-

public IView CreateView(IState state, string viewName,
                        string areaName)
{ 
  var view  = IoC.CreateObject<TView>(); 
  var viewModel = IoC.CreateObject<TViewModel>(); 

  viewModel.Load(state); 
  . . . 
  view.ViewModel = viewModel; 
  . . . 
  var area = ViewManager.Areas[areaName]; 
  area.AddView(viewName, view); 
  . . . 
  return view;
}

Now, the code is already doing quite a bit more than I would expect a factory method to do, but that is down to the framework having what I consider excessive demands.

The Test

As I said above I’ve had very little experience with Rhino Mocks, and this post is not about that product which is undoubtedly very clever and very useful. No, it’s about what is being tested with it (the syntax is probably slightly wrong but I hope you get the idea):-

[Test]
public void testCreateView()
{ 
  . . . 
  Expect.Call( viewModel.Load ); 
  Expect.Call( ViewManager.Areas[areaName] ); 
  . . . 
  var view = factory.CreateView(. . .); 

  Assert.That(view, Is.Not.Null); 
  . . .
}

Let’s put aside test naming conventions for now and focus on what’s inside the curly braces. The test is attempting to verify (amongst other things) the following two behaviours:-

• The state is loaded into the ViewModel object
• The view is registered with the view manager

The big problem I have with the test is that it is not verifying that the outcome of the test matches these expectations but that it is verifying exactly how those expectations are being met. In essence the feature specification appears to be saying this:-

• The state is loaded into the ViewModel object by calling the Load() method
• The view is registered with the view manager by associating the view with named area accessed via the indexer property

In short, why should the test prescribe the mechanism used to implement the feature?

Variations On a Theme

I’m going to discuss the “view registration” expectation first because I have another thing to say about the “state loading” that is more design related.

The expectation of the application framework is that the created view will already be registered with the “view manager” by the time the factory method returns. But we may be able to implement that in more than one way depending on what the interface to the view manager looks like. It’s not uncommon to augment a simple interface with different overloads (and extension methods) to allow the caller to write simpler code:-

viewManager.AddView(area, view);

vs

viewManager[area] = view;

You may also choose to introduce your own facade over the abstraction and therefore invoke it indirectly:-

// Does all 3 necessary steps
myViewManagerFacade.Register(area, name, view);

I’ll leave it up to you to decide which you prefer, but my point is that your test should not care which of these many forms are used. In fact it should be possible to switch between them so long as they still adhere to the contract. By not being overly specific in your tests you stand a better chance of not writing brittle tests that break in the face of minor refactorings. One such refactoring that I’ve done in the past is to simplify an interface by introducing extension methods to add syntactic sugar so that the interface remains as simple as possible; should I change the expectation to now be on the extension method or the new interface method? I don’t believe that you can mock extension methods and so now your test code looks inconsistent with your production code. If you introduce a facade, that itself is mockable, do you verify that interface call or the underlying one? From the test’s perspective the outcome is the same and that is what ultimately matters.

So what would my test be? Well, if the real view manager type brings too much baggage to allow me to use it directly in the test I’ll create a mock that will implement the bits I need, i.e. record the results of the interaction:-

public Dictionary<…> m_views = new Dictionary<…>();

public void AddView(string area, IView view)
{ 
  Views.Add(area, view);
}

In mocks I have absolutely no qualms about using public fields because the test is the important thing and the mock is a means to a different end than production code. Then in the test I just assert that the view was registered with the correct name:-

{ 
  var viewManager = new MockViewManager(); 
  var factory = new Factory(viewManager, . . .); 
  . . . 
  var view = factory.CreateView(state, . . .); 

  Assert.That(view, Is.Not.Null);  
  Assert.That(viewManager.m_views[area],
                                 Is.EqualTo(view)); 
  . . .
}

This is a manual mock and no doubt Rhino Mocks makes this kind of thing even easier to write if I just bothered to RTFM. The key point is that I’m only testing the outcome, not the means by which that outcome is achieved. I can change the factory implementation in the ways mentioned earlier and it in no way invalidates the test because from the application framework’s perspective I have met the contract.

Reinventing The Wheel

One thing that worries some people is that I’ve just re-implemented the ViewManager purely to test some other code and have therefore wasted my time re-inventing the wheel and created even more code that needs to be maintained. Well, yes and no. Note that I said I’d only do it if I couldn’t reuse the real ViewManager in my tests. Unit testing is about isolation and determinism – you don’t want a lot of dependencies making your tests brittle. If the real ViewManager cannot easily be plugged in by mocking its dependencies then yes I’d mock it, but if I can reuse it then no I wouldn’t.

When it comes to integration and system testing things get harder and at that point you may have to substitute more fully featured mocks to avoid taking a troublesome dependency on, say, an externally owned service. I may provide many mock implementations of the same service to suit the different types of automated tests being run.

Flexible Designs

So back to my other issue. The reason my colleague added the Load method was in part so that he could verify that the ViewModel object had been initialised from the persistent state. He suggested that in real world use the view model is unusable if this hasn’t happened and I get his point. But I would argue that it is not necessarily the responsibility of the factory to ensure that this happens. Once again we have to put ourselves in the shoes of the client (the application framework) and think how this would be observed – how can we interact with the resulting object in a way that depends on this interaction having being played out?

There are two ways to ensure that this orchestration happens:-

1. Explicitly, by the factory itself or some other mediator in the chain
2. Implicitly, as part of the ViewModel object’s construction

The first approach, which is the one my colleague had chosen, raises other questions such as what happens if someone calls Load a second time later? Or what behaviour can be relied upon between construction and Load being called? Two-phase construction is something I really try and avoid because when somebody does do the unexpected it can manifest as one of those hard to find bugs. And so I favour that latter, after all it’s called a constructor for a reason because it’s expected to construct an object :-).

The obvious follow-on question is “if I delegate responsibility of state loading to the ViewModel constructor then do I still need to test if the object returned from the factory is fully constructed?” Almost definitely “Yes”. There is a reason the method takes arguments and so you are probably expected to use them. If you have overloads of a factory method that do different things, say, one default constructs a view and the other constructs from an external resource you’d want to know that the objects were constructed in the appropriate way.

Now let’s add another common design choice into the mix – lazy loading. Does it matter if the state loading happens directly during construction, asynchronously in the background or even synchronously as some by-product of a method call that depends on the object having been initialised? The test should succeed either way because the object is either fully constructed up front or by the time the assertions attempt to verify the expectations.

Once again let’s compare the two approaches. The original test constrains the implementation into calling a specific method called Load, whereas the latter allows for a number of different implementations. It also allows the code to be refactored between the various implementations without affecting the tests.

Test Driven (Development|Design)

I think this kind of problem highlights the difference between the last word in TDD being interpreted as [D]evelopment or [D]esign. In the former you know what production code you’re going to write and therefore you bias the test towards verifying what you’re going to implement. But in the latter you don’t really know or care how it’s going to be implemented and so the test only verifies that the design allows for it to be done in some way. And the best way to achieve that is to only verify the results of any interactions and not the interactions themselves.

Using Extension Methods to Extend A C# Interface

When I first read about extension methods being added to C# (which was a long time before I actually started using C#) I thought they were a liability. Superficially it seemed to be “an accident waiting to happen” because it’s hard enough trying to understand the responsibilities of a class or interface when you have the code in front of you. With extension methods there could be behaviours bolted-on by a third party that you just can’t see.

Once more the classic quote from Spider-Man “with great power comes great responsibility” rears it’s head. Used judiciously extension methods are a great way to graft on functionality that a class author hasn’t provided, or a way to provide a Fluent Interface to a particular problem domain. I would hazard a guess that the String class has had significantly more extension methods than any other as I personally have a written a few obvious ones such as IsEmpty and JoinUsing.

Extending concrete classes probably makes a lot of sense, but extension methods can also be used to extend interfaces. That sounds a but more surreal – how do you extend an abstraction in a concrete way that applies to all implementations of that abstraction? There is nothing magical it’s really just turning the idea of an abstract base class on it’s head…

An Initial Configuration Mechanism

All non-trivial systems end up with some common configuration mechanism and my current one is no exception. Whilst bootstrapping the initial components I didn’t want to get into the nitty-gritty of the .Net ConfigurationManager class so I introduced a simple interface:-

public IConfiguration
{ 
  string Setting(string name); 
  string Setting(string name, string defaultValue);
}

The first overload will return the setting value or an empty string if there is no setting with that name[*]. The second overload returns the default value specified when there is no value to return. The natural implementation for the first overload then becomes:-

public string Setting(string name)
{ 
  return Setting(name, “”);
}

I tossed and turned about leaving the first overload out as it forces the implementer of IConfiguration to implement something that is really just syntactic sugar. Initially the only way round this that I knew was to introduce a default implementation so that the implementer just had to derive from it to get the syntactic sugar method:-

public abstract class ConfigurationDefault
                       : IConfiguration
{ 
  public string Setting(string name) 
  { 
    return Setting(name, “”); 
  }
}
. . .
public class XmlConfiguration : ConfigurationDefault
{ 
  . . .
}

The use of abstract base classes is a tried-and-tested approach to reusing code in a polymorphic class hierarchy and so I was happy. At least until some new requirements came along.

Improvements to the Configuration Mechanism

The implementation served us well to start with but I knew from experience that there are certain behaviours I’ve found really useful, such as being able to specify environment variables like %ProgramFiles% and %TEMP% in file paths that are then expanded on demand. By now I had started getting to grips with extension methods and quickly realised that this was a behaviour that was also independent of any IConfiguration implementation, so I wrote it as an extension method:-

public static class IConfigurationExtensions
{ 
  public string PathSetting(this IConfiguration 
      configuration, string name, string defaultValue) 
  { 
    var setting = Setting(name, defaultValue); 

    // Do substitutions 
    . . . 
  }
}

One Method to Serve Them All

After adding this support and a further behaviour to allow arbitrary substitution of other settings I found that it was all too easy to mess one up and get odd behaviour. We decided that the silent failure when doing substitutions was a bad idea and that instead we’d prefer to throw an exception if a setting or environment variable did not exist. But that caused a problem because the original interface was not designed to allow the absence of a setting to be detected, it just returned an empty string or the default value and I didn’t want to get into passing a random default value just to detect if the setting existed or not.

At first I thought that I should just extend the interface with a TryXxx() style method that would return true or false along with the value if it existed. But it was whilst thinking through the ramifications of this change that I realised that this was in fact the only method that I needed in the interface:-

public IConfiguration
{ 
  bool TryGetSetting(string name, out string value);
}

All the other methods could be written as extension methods implemented in terms of this one interface method:-

public static class IConfigurationExtensions
{ 
  public string Setting(this IConfiguration
                        configuration, string name) 
  { 
    return Setting(name, “”); 
  } 

  public string Setting(this IConfiguration 
      configuration, string name, string defaultValue) 
  { 
    string value; 

    if (configuration.TryGetSetting(name, out value)) 
      return value; 

    return defaultValue; 
  } 

  public string PathSetting(this IConfiguration 
      configuration, string name, string defaultValue) 
  { 
    . . . 
  }
}

This also meant that I could drop the abstract base class that provided the default implementations of the syntactic sugar methods.

OK, so I haven’t exactly turned the abstract base class on its head, I’ve really swapped an abstract base class for a static helper class and with that I have lost the ability to re-implement the methods another way in the derived classes. But the whole point of the base class was not to enable polymorphism but to provide reusable helper methods – inheritance was just a means to a different end.

My original fears about extension methods lay unfounded and instead have provided a much cleaner solution to the problem of extending an interface to provide syntactic sugar (e.g. a fluent interface) where there previously was none. This revelation will no doubt be nothing new to experienced LINQ users because that’s exactly what it provides for IEnumerable, but for me it’s added a new dimension to how I think about and design my own interfaces.

 

[*] Yes I could have returned ‘null’ but my C++ roots were telling me that I should return a value not an empty reference (or NULL pointer in the C++ world). I still struggle with this duality of the String type – null vs “” – although as I start to use nullable value types more in the Data Access Layer I feel I’m getting to grips with it slowly.

You Write Your SQL Unit Tests in SQL?

One of the questions that came up after my ACCU London talk back in January (xUnit Style Database Unit Testing) also came up last week after my ACCU Conference talk (Using xUnit As a Swiss Army Testing Toolkit):-

Why do you write your database unit tests in SQL?

My immediate and somewhat flippant response is:-

Why wouldn’t you write your SQL tests in SQL? You wouldn’t write your Python unit tests in C++ would you?

Now, if you swap the words Python and C++ around in that sentence it begins to sound more plausible. In fact it’s an idea that I’ve looked into in the past, but not so much at the unit test level, more at the component/integration level. Kevlin Henney, in his ACCU London talk last year, pointed out that you could use NUnit[*] to unit test C++ code via the magic of C++/CLI so using a different language to express your tests is by no means wrong but it comes with another cost…

Portability (People Skills)

Steve Love did a talk at the ACCU Conference back in 2009 about portability. Of course what most people think of when you say portability is about the source code and how it works across multiple platforms (e.g. Windows/Unix) or across multiple toolchains (e.g. Visual C++/GCC). But there is another aspect to it and that is portability across people – can you hire another person to do the same job?

I’ve worked in small teams where everyone has to do everything and also in larger teams where people specialise in a particular language or technology. It is hard enough to find good people as it is, adding another orthogonal requirement to the role only makes the search that much more difficult. OK, so many experienced T-SQL developers will probably also have some knowledge of C# and the Cool Kids will happily remind you how we should all be Polyglot Programmers and be able to work with multiple languages, but the organisations I work in don’t get to attract the superstar programmers. Sometimes you’re ‘given’ members of staff from another team, presumably because they can save money by avoiding firing one and hiring another in the mistaken belief that you will turn them into a superstar programmer.

Isolation (Tooling)

Even so, just because they might know C# doesn’t meant they feel comfortable using, say, Visual Studio as their main development tool. Our automated test runner currently uses SQLCMD and we (can) write our SQL code and tests using just SQL Server Management Studio (SSMS). This means that not only are the tests expressed in their language of choice, but the tooling is also the one they are likely most comfortable with. I’ve not tried SQL debugging with SSMS but I would have thought that it is far simpler if you’re not trying to debug across a high technology stack (it feels akin to debugging across RPC calls).

That last point sums up my approach to unit testing, which is that it is all about isolation. That doesn’t just apply at the code level – at the thing under test – but also to the toolchain that sits around it. That doesn’t mean that you should only use primitive tools like Notepad & SQLCMD to develop your code, but that you try and avoid having too many other layers (e.g. C#, ADO.NET) ‘above’ the code in your test scaffolding as it makes it unnecessarily complicated and can make it harder to do things like debugging.

The Right Tool For the Job

Deciding what the right tool is for any job is getting harder every day because new tools are springing up all the time that bring the best bits from other tools together to solve some other hybrid problem. I’m definitely sold on the idea of writing SQL unit tests in SQL as the language provides enough features to allow you to write fairly clear tests, but our Heat Robinson style test runner is clearly begging to be replaced by something better. I also need to look again at how Visual Studio studio approaches the problem because they have an army of developers [hopefully] much cleverer than me that should be able to balance these requirements. I also promised myself I’d got back and look at how TSQLUNIT was coming along as that is also more mature than our framework.

We never set out to write a SQL unit test framework, we just did what felt natural to bootstrap ourselves and that’s the way it ended up. Looking back I still think it feels the right way to go.

[*] Whereas other languages seem to have a few key unit test frameworks, C++ is blessed with a bucket load. At the ACCU Conference last week someone asked for a show of hands of those people that had written a unit test framework and there were quite a few hands. I’ve said before that I believe it’s somewhat of a Rite of Passage and writing a unit test framework seems to be common part of that voyage of discovery.

Database Enumerations – A Non-Performant Query

Back in September last year I wrote about how we have implemented constants and enumerations in our SQL Server database (Implementing Constants & Enumerations in a Database). Essentially we have used parameterless User Defined Functions (UDFs) to act as the symbol for the constant or enumeration. At the time I said that I was not aware of any performance impact that this had, but that I would write it up when I found one. Well, I wrote a support query the other day and noticed that there was quite a difference between the query with and without the UDF. It looked something like this:-

SELECT *
FROM   Requests r
JOIN   Tasks t
ON     r.RequestId = t.RequestId
AND    t.TaskType = dbo.TaskType_Calculation()
WHERE  r.ValueDate = . . .

The two tables involved in the join have millions of rows in them and although there is an index covering part of the query the selectivity is quite low in this case. I didn’t look at the execution plans because this is a template query that I use regularly and the only difference was the explicit filter on TaskType using the UDF. The query took 13 seconds to execute.

Given how often I run very similar queries to this I suspected this was the evidence I was looking for to show how a UDF could affect query performance, and so I did the obvious thing which was to substitute the call to the UDF with the relevant constant:-

AND    t.TaskType = 2 -- dbo.TaskType_Calculation()

Lo and behold the query ran in just 2 seconds. I switched back and forth between the two forms to see if the performance was consistent and it was – 2s vs 13s. Somewhat perturbed now that our lovely idea may be an accident waiting to happen I checked the performance using an intermediate variable, which is effectively how we have used this idiom in our code (because you can’t always reference the UDF directly and it’s a bit verbose):-

DECLARE @calculationTask udt.TaskType_t
SET     @calculationTask = dbo.TaskType_Calculation()

SELECT *
FROM   Requests r
JOIN   Tasks t
ON     r.RequestId = t.RequestId
AND    t.TaskType = @calculationTask
WHERE  r.ValueDate = . . .

Tentatively I pressed CTRL+E in SSMS to execute the query… and… it returned in just 2 seconds. Just to be sure I then alternated between all 3 forms but the intermediate variable version still performed exactly the same as the hard-coded constant. Phew! This is clearly not an exhaustive test but it does give me some continuing comfort that it is a sound idea but that we now know for sure that it can have a performance impact in some scenarios.

P.S. This was on SQL Server 2008 R1.

Describe the Problem, Don’t Prescribe the Solution

The other day whilst scanning the backlog of change requests in JIRA I came across the following entry:-

Replace the use of nchar(x) with nvarchar(x)

And that’s all it said[#]. Now I knew instantly which tables the author was referring to because, like many teams, we only use a fixed width char(x) column for flag type fields such as Y/N or true fixed width strings like currency codes. The reasons why and how these couple of table definition slipped past the informal code review process is somewhat irrelevant, but suffice to say that we were days away from release and chose to take the ‘hit’ instead.

The fact that variable width text was being stored in a fixed width column was not causing any problems within the database or the back-end server code. Yes it was annoying when you cut-and-pasted result values from SSMS (SQL Server Management Studio) as you got a load of trailing spaces that you just had to manually trim:-

...WHERE t.Column = ‘VALUE     ‘

So I put this one aside mentally filing it with the other issues of Technical Debt as there was nothing to fix per-se – the mechanism was ugly but it worked. There was also no reason to suspect it may cause any performance problems either as the tables contain relatively few rows of data.

Then a conversation some weeks later with the change request submitter brought up the subject of raising Technical Debt style change requests and I opined that now we were live the database schema is potentially a whole lot harder to change because we have data to migrate as well[+]. In this instance it wasn’t even vaguely in the area of rocket science, but nevertheless it was non-trivial and ultimately had to be paid for. And then the real requirement came to the surface and brought with it a new perspective…

These fixed width columns were not causing the back-end any problems because we never consumed them – we only every compared them to string literals or ran support queries. The impending GUI on the other hand had controls that displayed some of these fields and apparently the UI framework would bloat the size of the control to allow room for what it presumably thought were always going to be long textual values. My gut reaction was abhorrence at the thought of changing the database schema of a base table just to fix a layout issue in the GUI; that is the kind of nasty coupling that causes change paralysis because managers get worried about what will break. I understood it was a pain having to manually deal with the fallout in the GUI layer but there should be other much simpler solutions that didn’t involve writing SQL scripts to fix the table schema and migrate the data – namely trimming the string somewhere in the data access layer or the SQL query used to fetch the data[*].

Hold on a second I thought. What are you doing directly accessing base tables anyway? The rest of the public interface to the database is via stored procedures and views – you’ll need to abstract that access away behind one of those before going to production. More importantly that gives us another option for remediating this behaviour without touching the schema and data, and better yet, that would be inside the database public interface and so allow the refactoring to occur without changes to any clients. Of course this then forces the issue of what the public interface to this mechanism should really be and at that point it seems prudent to design it properly if the existing implementation is going to leak out badly. But if it doesn’t we can save that job for another day.

The important thing though is that we have identified the root cause and can therefore now begin to assign some value to the potential choices and costs. When the request implies changing the schema of persistent data purely for the sake of it, the risk seems high and costly and the reward seems low. But when faced with a path that allows for incremental change so that the full cost and risk is distributed over smaller steps it seems much more appealing as you can put your spade down at any point.

So how would I have initially phrased that change request? Well a few weeks ago I would have gone for something like this (I can’t remember what the exact issue with the GUI was so it still seems somewhat lame I’m afraid):-

Laying out the XYZ controls has to be done manually because the ABC data has extra whitespace padding due to the use of nchar(x) instead of nvarchar(x) for the FGH columns. These columns should probably be nvarchar(x) anyway as they contain variable width text.

However a recent article by Allan Kelly in the ACCU magazine Overload (The Agile 10 Steps Model) contained a sidebar titled “What’s the Story?” about User Stories. This described a common format for stories:- “As a [Role] I can [Action] So that [Reason]” that felt akin to the use of “[action] [should] [when]” that I follow when writing test case names (itself a variation of the “[given] [when] [then]” style). I felt that this would allow those of us less natural writers to still construct a succinct requirement even if it does sound somewhat robotic, but more importantly it would help bring the value of the request to the forefront. User Stories is something that I’m vaguely aware of but have largely glossed over because my requirements are nearly always technical in nature and tend to come through very informal channels so any connection with ‘End Users’ is usually missing; it just hadn’t clicked that “User Stories” could just as easily be read as “Developer Stories” or “Support Stories”. So this is my attempt at writing it this way:-

As a [developer] I can [trim the whitespace from the ABC data] so that [the XYZ controls are not laid out manually].

OK, so this reads very unnaturally because the story format implies a repeatable action rather than a one off event. A small and hopefully obvious change to the wording and order though should make this more natural:-

As a developer it would save time if the XYZ controls were laid out automatically by ensuring the ABC data contains no excess whitespace.

Do you think that reads better or not? I think the value, while still somewhat intangible as all technical requirements like this are, is clearer because you are forced to justify the action rather than providing it as an afterthought. The beneficiary of the requirement is also clearly stated which helps easily separate the functional from the non-functional.

Of course what the original submitter would probably have written would still be something like this:-

As a developer I need to replace the nchar(x) columns with nvarchar(x) columns so that the GUI layout is automatically laid out.

…because the [action] would likely still be the same, but one would hope that when writing the [reason] alarm bells should start ringing as the tight coupling between the database schema and GUI becomes apparent.

 

[#] This is also a common failing of n00b posters on sites like Stack Overflow. They have already decided on what their solution is and are trying to elicit an answer that helps implement that solution rather than taking the time to explain the underlying problem in case the reason they can’t see the proverbial wood is because there are a load of trees in the way.

[+] As you will see later our well defined SQL public interface and set of SQL unit test means that it is actually quite easy to refactor the schema.

[*] Eventually an O/RM such as Entity Framework or NHibernate will no doubt be in place to tackle this, and so it will be interesting to see how you would go about dealing with this kind of problem when you don’t immediately have access to either the SQL or the code that binds the result values to the DTO members. I’m sure it is easy, but how much out of the way would you have to go?

Friends Don’t Let Friends Use Varargs

Whilst writing my previous post about being clever with strings and varargs functions I was reminded of another time that I had a run-in with them. Luckily I wasn’t the protagonist this time but a colleague instead. He decided that he wanted to make it easier to add an arbitrary set of strings to a particular class via the ctor. In modern day C++ you might do it via an iterator pair like this:-

{
  const char* words[] = { “cat”, “mat”, “dog” }

  Thingy thingy(words, words + ARRAYSIZE(words));
  . . .
}

However this was many years ago and the project was MFC based so the use of STL idioms were very thin on the ground[$].

Another less obvious choice might be to define a vector, add them to that and define a ctor that takes a vector of strings. But nope, my colleague decided that he could avoid the ugly array definition and performance hit of the vector approach by declaring a varargs ctor like so:-

class Thingy
{
public:
  Thingy(const char* word1, ...);
};

This meant that you could provide an arbitrary parameter list of strings and then terminate it with a NULL parameter:-

{
  Thingy things(“cat”, “mat”, “dog”, NULL);
  . . .
}

Naturally I suggested you don’t mess with varargs and that this was an accident waiting to happen[#]. I did agree that it was not an uncommon technique to define an array of items and use a NULL terminator at the end and that what he was effectively doing was this:-

{
  const char* words[] = { “cat”, “mat”, “dog”, NULL }

  Thingy thingy(words);
  . . .
}

At the time we had a definite need to support up to three arguments which were actually all CStrings. I’m sure I suggested writing three ctor overloads that then call a common internal helper method instead for now:-

class Thingy
{
public:
  Thingy(CString arg1);
  Thingy(CString arg1, CString arg2);
  Thingy(const CString& arg1, . . . arg2, . . . arg3);
};

But no, that was far too much work, his way was simpler and, after all, we were all experienced programmers so we don’t make those kind of silly mistakes…

Sadly I can’t remember when the first bug showed up or why it remained hidden for so long[+], but one did suddenly appear and guess what was missing? Yup, the NULL terminator. Of course now I had to check for any other occurrences of malformed ctor calls and I soon discovered one more. Clearly someone was not aware of this need to add a terminator so I did a little Software Archaeology and rooted around in our VCS to find out who needed a little educating… But of course, it was my esteemed colleague, the very same person who suggested that we wouldn’t make that kind of mistake!

So remember kids – Just Say No to Vargs Functions. Unless of course you’re doing Template Meta-Programming in which case you’ll be [ab]using them for a different reason.

 

[$] Due to it’s age MFC has its own containers and single iterator type and doesn’t adhere to the STL idioms that all all know and love now. Consequently if you spent your years knee deep in MFC code you could very easily to be unaware of them.

[#] Of course this was many years before I became aware of my own misfortunes with varargs function and anyway I’d read all the books by Scott Meyers, Herb Sutter, et al so that made me the fountain of all knowledge, right?

[+] It just goes to show that even with debugging techniques like the compiler using guard blocks and filling uninitialized stack space with a non-zero pattern it’s still possible for the stack to end up correctly formed by luck and not by judgement. That’s especially true when the stack variable is a ‘bool’.

A Not-So-Clever-Now String Implementation

Back around 1995 when I first started using C++ commercially[+] I picked up an excellent book by the late Paul DiLascia titled “Windows++”. It was about writing Windows applications in C++ and the book developed a framework along similar lines to the then fledgling MFC. I decided that writing my own C++ framework would be a great way to learn C++ and I (somewhat foolishly) decided that I would use a similar code style to MFC so that I could easily reuse any of my framework code in my day job (after making it production ready of course).

Reinventing the Wheel

The heart of any C++ class library back then was the String class. C++ was still evolving and my work involved using MSVC 1.52c (aka Visual C++ 1.5) which had no template[^] support so naturally I created my own ‘classic’ class that managed an ANSI char buffer:-

class CString
{
private: 
  int   bufsize;  
  char* buffer;      
  . . .
};

One of the problems with this class was that it was too easy to screw up[*] when using the printf() family of functions because it had a different memory layout to a plain char*, so if you forgot to invoke the moral equivalent of c_str() you could end up crashing the process because the bufsize member would be interpreted as a string pointer:-

{ 
  CString str = “chris”; 
  printf(“hello %s”, str); // BOOM!
}

Swapping the two members around would probably work for the single-string case above, but not for N arguments as the trailing bufsize member would become the next string:-

{ 
  CString first = “chris”, last = “oldwood”; 
  printf(“hello %s %s”, first, last); // Still BOOM!
}

Now, I felt that giving up the beauty of the printf() family was too much; I had just spent the last few years learning C and so I had the various format specifiers committed to memory. Windows also had its own variation, wsprintf(), that supported a few other useful extensions such as near/far pointers and the ANSI/Unicode build dependent variants of “%c” and “%s”.

Tempted by the Dark Side

One day whilst debugging, I stepped into some code that passed a MFC string directly into a printf() call and realised that it hadn’t gone bang. As I stepped into the printf() call I realised that the reason it had worked was because an MFC CString object had the same stack footprint as a raw char* pointer. When an object is passed into a varargs style function the object is passed by value, which in the case of a CString is effectively a single pointer. Also because the class only has a pointer based member it has no extra padding or alignment requirements – at least not on the x86 targets I was interested in.

I thought this was rather clever and set about changing my implementation so that it had similar behaviour. It hadn’t registered at first but I later realised the similarity with the implementation of the COM BSTR type which also stores the string length in an area preceding the string contents. And so I switched my implementation to this:-

class CString
{
private: 
  class CStringBuffer 
  {  
      int   bufsize; 
      char buffer[0]; 
  }; 

  char* string;  // Really CStringBuffer.buffer 
  . . .
};

So now I allocate a single chunk of memory for the header (CStringBuffer) + contents (N * sizeof(char)), but instead of storing a pointer to the CStringBuffer header as a member, I slide along the memory block and store a pointer to the first character of the string, i.e. &CStringBuffer.buffer[0]. I used an interesting Microsoft extension- the zero element array. This beast used to be quite common in the Windows headers and is a useful way to create a placeholder for a variable length array that follows a structure in a contiguous piece of memory. The alternative is the standards compliant “char buffer[1]”, but then that makes your sizeof() arithmetic a little more complicated as you have padding to account for along with the extra byte in the array. Of course to access the header structure without littering the code with casts I wrapped the necessaries in an inline method:-

CStringBuffer* CString::GetBuffer()
{ 
  return ((CStringBuffer*)string)-1;
}

All very ugly and switching from a C-style cast to a proper reinterpret_cast<> doesn’t make it any less unpleasant. Still, I happily lived in ignorant bliss for well over a decade until I decided to make a conscious effort to drop my CString class in favour of the standard string type std::string – at least for new libraries that is. The conduit was reading Matthew Wilson’s Imperfect C++ and coming across his STLsoft libraries. Suddenly I found a new impetus to deal with the impedance mismatch between MFC/ATL and Standard C++ (something I believe his new book may well be tackling).

A New Hope

The switchover went swimmingly well with my new Core and XML libraries showing the way forward whilst the old WCL, NCL and MDBL libraries became the epitome of the word ‘'legacy’. Where possible I added extra overloads to allow std:string’s to be used naturally but by-and-large the worlds were kept apart and the cost of the occasional implicit[#] conversion from a CString to a std::string was left for the profiler to find, should it ever show up.

Then I decided that where possible all new code, even in my legacy libraries, should adopt the proper string type and that I would slowly migrate code across. So I started making deeper and deeper refactorings (backed by my unit tests – where they existed) and still things looked good. Visual C++ 10 (aka VS2010) arrived on the scene and so a port was required to ensure my codebase was compatible and much to my surprise a unit test in my ANSI to Unicode conversion helper functions choked on an ASSERT inside the STL. The code was wrong, no doubt about it, but why hadn’t I see it before as I usually use STLport to keep me honest? I did a build with STLport 5.2 and it also whinged! So I fixed my iterator dereferencing bug and ran the entire suite to make sure I hadn’t entered into the murky waters of Undefined Behaviour anywhere else by accident and again it when BANG! One of my COM library tests was now failing! Running the test under the debugger revealed a more serious problem as the MS debug runtime started complaining about other matters…

Undefined Behaviour Strikes Back

It took a couple of debugging attempts but then I winced as I spotted my error. I had switched over a couple of helper functions from returning a CString to a std::string and the result of that function was in turn being passed to a varargs logging function like so:-

std::string SomeClass::toString()
{ 
  return . . .;
}
. . .
void Elsewhere::DoSomething()
{ 
  LOG(“Blah, blah, blah %s”, something.toString()); 
  . . .
}

Yup, there was no call to c_str()!

Then the true cost of my foolish design choice started to dawn on me as I realised that there was no obvious way to catch the problem at compile time because varargs functions are effectively type-less once you extend into the ‘variable’ part of the parameter list. Yes, it might be possible to do something with templates and macros but that would be non-trivial. Add in the fact that I have used varargs style functions to do all my string formatting, often in my exception handling, and you can guarantee that there are plenty of time bombs in the shape of rarely executed exception handlers just waiting to go off.

Another New Hope

Maybe it was fate, but I had always been meaning to get my Core library and its unit test suite to build and run under more than just Visual C++. So I started investigating the alternatives such as GCC and Digital Mars (see “GCC for the Visually C++ Impaired”) and soon I had both Core and XML working under GCC 3.x and 4.x via the Code::Blocks IDE – albeit only with the standard warning level.

After being distracted for some months I finally got back to playing with GCC as I had always intended on compiling my code with the highest warning settings available to see how much static code analysis GCC can do over-and-above what VC++ already does – an awful lot it turns out (a topic for another day). And, guess what one of the behaviours that it singles out? Yup, the passing of non-primitive types to printf() style functions! I’ve yet to dig deeper and see if it is varargs functions in general or whether it’s functions like printf(). Either way it spots code like this:-

{ 
  std::string text(“. . .”); 
  printf(“%s”, text);
}

Which is great because I now have a clear path forward for deprecating my CString type safely:-

1. Add a c_str() method to my CString type to provide compatibility with std::string
2. Start compiling[$] more of my codebase with GCC 4.x to identify and fix those bugs by appending a call to .c_str()
3. When a suitable refactoring moment arises switch code from my CString type to the standard string type
4. Test, test and test some more…

Epilogue

I often wonder whether it’s still worth maintaining my 15 year old C++ codebase of tools. Many of them were made obsolete years ago and so I have stopped fixing up the really old applications but the libraries live on – kept alive by each new utility that I write. I could start again from scratch but you don’t get that luxury in the real world so I’m inclined to try and refactor my way out. This way it remains a learning exercise – albeit one about refactoring legacy code – which sadly is what a lot of commercial C++ projects have become.

 

[+] I first used it at university some years earlier and at that time you used the ‘overload’ keyword to mark overloaded functions. I managed to get G++ (nay GCC) working on my Atari 1040 STE (with twin floppy drives) but it was not a pleasant experience. I already thought C was too bloated in comparison to Assembly Language and C++ was therefore even more pointless. Sooo naive…

[^] Not that I knew what a template was back then of course. I never really started to understand templates until the turn of the millennium and Template Meta-programming is still largely theory to me – that’s why there’s the Boost libraries!

[*] If you were lucky it was in a common code path and you got a UAE/GPF there and there; but often you weren’t and it became a time bomb if the code path was in an error handler that was called infrequently.

[#] Yes, I added a conversion operator to ‘const char*’ to avoid having two overloads on every text API method or littering my code with .c_str() style warts. Choose your poison…

[$] Sadly, although my Core and XML libraries link and I can therefore run the unit tests, I can’t do the same for my WCL, COM or MDBL libraries or projects dependent on them (i.e. everything). But that’s ok because it’s the static code analysis I’m after anyway.