Don't shoot the reviewer

Introduction

Recently while reviewing some code that a colleague has written I made a fairly innocuous comment (or so I thought). The code was a series of nested switch statements and looked something like this

switch (param->type)
{
    case LINE_SPEED:
        switch (param->data)
        {
            case BAUD_19K2:
            retVal = "19200";
            break;
            case BAUD_9K6:
            retVal = "9600";
            break;
              .    .    .    .
              .    .    .    .
              .    .    .    .
        default:
            retVal = "2400";
        }
        break;

    case LINE_PARITY:
        switch (param->data)
        {
            case PARITY_EVEN:
            retVal = "EVEN";
            break;

            case PARITY_ODD:
            retVal = "ODD";
            break;

        default:
            retVal = "NONE";
        }
        break;

    case BUFFER_LEN:
              .    .    .    .
              .    .    .    .
              .    .    .    .
}

The code fragment was translating between configuration values stored in non volatile memory and their textual equivalents for Display to the user. The intent was clear enough it was just that there was too much of it. I’ve only shown three of the twenty or so parameters and already it occupies over 40 lines and that was after heavy precis.

Extrapolating this for all parameters would lead to over 250 lines of code. That’s way too much just to translate between enums and ints.

To make matters worse there was a similar sized function to translate the other way ie from strings like “19200″ to enums line BAUD_19K2

So my simple one line comment (remember that) in the review sheet was

Can’t all this be replaced by a couple of maps?

Thinking that I’d saved us a lot of code and maintenance, I finished the review and got on with my own coding. A couple of days later the engineer whose code I had reviewed came over to me and asked if I could explain what I meant by the comment.

Now he is an engineer with many years experience so I was a little taken aback. Surely he understood the replacement of a switch statement with a map? So I repeated the concept a simply as I could, halfway through he said

Yeah, I understand what you meant but I’ve tried for the last couple of days and I can’t seem to find a way of implementing it elegantly.

After the initial surprise we sat down together and went through it again.

The trouble was that I’d seen a much simpler problem than was really there. Once I’d been seduced by the idea that we coule add new parameters to the mapping and not have to modify the code, that was it I was hooked.

My mind had raced ahead and as I thought about the possibility of autogenerating the inverse maps for translating the other way round, I was drunk on inspiration, it (I) was brilliant.

But then, in the cold light of day, like any hangover it hit me. A set of nested switch statements is like a two dimensional map, that is pairs of keys (parameter type and data item) map to a single data item. Did std::multimap do that ? I didn’t think so. I checked, no luck.

Still maybe we could create a composite key out of the parameter and its data item, or maybe a two stage map, one to map the parameter to another map which then maps the data to it’s text

Regardless of this, there was another issue that was bothering my colleague. How do we initialise all these maps ? Currently the translation was performed inside a Configuration class and all was fine because the configuration code is specific to the application and so burying them inside here was okay.

But the solution I had proposed was much more general and should in theory be reusable across many projects. Burying all the map initialisation inside the Configuration class would prevent such reuse.

It would have to be passed in but how. One solution would be to construct the map outside of the class and then pass it in but this defeated the idea of the class as a data abstraction mechanism. It should be up to the controlling class to decide how the data is stored and managed.

In the end we decided that while the “map thing” had potential we would have to defer it on the grounds of time and put it on the TODO list for when we had time at the end. So in the spirit of the song by the Police, it was an idea that I’d loved, so I let it go. Later that evening, it came back to me

It kept gnawing at my conscious, like a puppy nibbling at my toes. I couldn’t get the original idea out of my head. There must be a way to recode this using maps, elegantly.

What we had originally was a Configuration object that managed a distinct set of configuration items internally. What we needed was to pass these configuration items in. But in order to make the Configuration generic, it had to have no knowledge of how many items there were.

The constructor seemed like the obvious place to “convey” the information to the object, however because there were an unknown (in the general case) number of items, it was almost like we needed a varargs interface. Was that it? surely not.

I thought about this for a while and wrestled with the compiler, but no matter how I tried I couldn’t make it accept a variable yet bounded set of parmeters without explicitly passing the number of parameters to the constructor

After thinking about this some more, it suddenly struck me that what I wanted was similar to how std::cout works – an appendable object. Maybe that was the solution. The stream insertion operator (<<) that returned a reference to itself enabled an unbounded set of items to be chained together.

The question was, could I make this work with our existing system. I tried some prototyping.

I created a Pair class with holds a numeric value and it’s corresponding textual equivalent. I then created a class Mapstream which implements the insertion operator for Pair objects and adds them to an internal map. This object can then be passed into the constructor of ConfigurableItem which can query the Mapstream for it’s value/text pairs.

MapStream msl;
msl << Pair(BAUD_19K2, "19200") << Pair(BAUD_9K6, "9600");
ConfigurableItem *baudRateType = new ConfigurableItem(eBAUD_RATE_TYPE, msl);
MapStream ms2;
ms2 << Pair(PARITY_EVEN, "EVEN") <<  Pair(BAUD_ODD, "ODD");
ConfigurableItem *lineParityType = new ConfigurableItem(eLINE_PARITY_TYPE, ms2);

Additionally by using the temporary object creation facility of C++ we can combine the creation and initialisation of the MapStream along with the call to the ConfigurableItem constructor

ConfigurableItem *baudRateType = new ConfigurableItem(eBAUD_RATE_TYPE, MapStream() << Pair(BAUD_19K2, "19200") << Pair(BAUD_9K6, "9600"));
ConfigurableItem *lineParityType = new ConfigurableItem(eLINE_PARITY_TYPE, MapStream() << Pair(PARITY_EVEN, "EVEN") <<  << Pair(BAUD_ODD, "ODD");

The calls to MapStream() create an anonymous instance of a MapStream which exists until the ConfigurableItem constructor returns, when it is destroyed. This is simpler and cleaner than creating the MapStream objects outside of the constructor and it avoids polluting the namespace with lots of temporary (uniquely named) MapStreams.

A working example of this concept is available here