Replace giant switch statement with what?

Asked 18/3, 2009 at 18:38 Answered 18/3, 2009 at 19:31

Solved c++templates switch-statement chain-of-responsibility

I have a code that parses some template files and when it finds a placeholder, it replaces it with a value. Something like:

<html>
<head>
    <title>%title%</title>
</head>
<body bgcolor="%color%">
...etc.

In code, the parser finds those, calls this function:

string getContent(const string& name)
{
    if (name == "title")
        return page->getTitle();
    else if (name == "color")
        return getBodyColor();
    ...etc.
}

and then replaces the original placeholder with returned value.

In real case, it is not a dummy web page, and there are many (50+) different placeholders that can occur.

My code is C++, but I guess this problem exists with any language. It's more about algorithms and OO design I guess. Only important thing is that this must be compiled, even if I wanted I couldn't have any dynamic/eval'd code.

I though about implementing Chain of Responsibility pattern, but it doesn't seem it would improve the situation much.

UPDATE: and I'm also concerned about this comment in another thread. Should I care about it?

Diffractometer answered 18/3, 2009 at 18:38 Comment(0)

Use a dictionary that maps tag names to a tag handler.

Adolfo answered 18/3, 2009 at 18:41 Comment(11)

Especially good if your dictionary can use a O(1) lookup like hashing. – Gimmal 18/3, 2009 at 18:43

Great answer. A tad on the lean side, but definitely a good way to go. : ) – Charnel 18/3, 2009 at 18:44

@paul I don't think the the performance is often an issue here - the clarity of the resulting code is more important – Adolfo 18/3, 2009 at 18:45

@ejames I've never understood why long answers are considered better than short ones – Adolfo 18/3, 2009 at 18:46

What should I use for second parameter to map? map<string, what?> – Shiller 18/3, 2009 at 18:47

Simplest would be a map<string, string>. – Boyceboycey 18/3, 2009 at 18:48

@milan - up to you - could be an instance of a class, a function pointer, a class on which you call a sttaic method... – Adolfo 18/3, 2009 at 18:48

Otherwise, you can go with maybe map<string, TagHandler> where TagHandler is an abstract class that you inherit from to handle the various template keywords you are looking for. Populate the map once at the beginning, and then use dictionary lookup. – Boyceboycey 18/3, 2009 at 18:50

@Neil Butterworth Short answers with helpful comments are really just long answers that require more clicking... :) – Bates 18/3, 2009 at 18:52

@Josh: Good idea, but that type needs to be map<string, TagHandler*> -- i.e. you must use pointers to the base class. Otherwise you will get object slicing instead of polymorphic behaviour. – Kuehnel 19/3, 2009 at 4:27

And where is the compile time checking? – Parallelize 23/3, 2009 at 7:55

You want replace conditional with polymorphism. Roughly:

string getContent(const string& name) {
    myType obj = factory.getObjForName(name);
    obj.doStuff();
}

where doStuff is overloaded.

Milquetoast answered 18/3, 2009 at 18:43 Comment(5)

Of course, the switch is just being moved somewhere else (the factory), which is where it should be. – Verjuice 18/3, 2009 at 18:45

You'd probably want to combine the factory with Neil Butterworth's map and load the instantiation logic from some configuration file. Compiled and dynamic -- amazing. – Milquetoast 18/3, 2009 at 18:47

Actually, that template IS the configuration file. Users can alter it themselves. – Shiller 18/3, 2009 at 18:59

I meant "configure the mapping of names to behaviors" (listed above as myType). Then there will be no switch involved. – Milquetoast 18/3, 2009 at 19:7

Ummmm... would that be "obj.doStuff()"? – Fendley 18/3, 2009 at 20:50

Have you considered XSLT? It's very well suited to this kind of thing. I developed a content management system that did the exact same thing and found XSLT to be very effective. The parser does a lot of the work for you.

UPDATE: Steven's comment raises an important point- you'll want your templates to be valid XHTML if you decide to go the XSLT route. Also- I would use a different delimiter for your replacement tokens. Something less likely to occur naturally. I used #!PLACEHOLDER#! in my CMS.

Danettedaney answered 18/3, 2009 at 18:44 Comment(1)

I think it's pretty optimistic of you to think that HTML templates will be valid XML. :) – Milquetoast 18/3, 2009 at 18:46

i'll combine 3 ideas:

(from Steven Hugig): use a factory method that gets you a different class for each selector.
- (from Neil Butterworth): inside the factory, use a dictionary so you get rid of the big switch(){}.
- (mine): add a setup() method to each handler class, that adds itself (or a new class instance) to the dictionary.

explaining a bit:

make an abstract class that has a static dict, and methods to register an instance with a selector string.
on each subclass the setup() method registers itself with the superclass' dict
the factory method is little more than a dictionary read

Ambient answered 18/3, 2009 at 19:1 Comment(1)

+1. I'll suggest getting rid of the separate setup() function and moving its behaviour to the constructor -- that way it can't be forgotten. – Kuehnel 19/3, 2009 at 4:31

Rather than parsing, have tried just reading the template into a string and then just performing replaces.

fileContents = fileContents.Replace("%title%", page->getTitle());
fileContents = fileContents.Replace("%color%", getBodyColor());

Mordacious answered 18/3, 2009 at 18:44 Comment(5)

performance hit, but probably worth it for code simplicity if absolute efficiency is not absolutely needed. +1 – Dyedinthewool 18/3, 2009 at 18:48

If "titleValue" variable contains the string "%color%" it would not work properly. – Shiller 18/3, 2009 at 18:49

ya it is less safe as well, but more simple still :) – Dyedinthewool 18/3, 2009 at 18:53

Yeah, probably better to make the tokens slightly more complex, like "[%color%]". – Mordacious 18/3, 2009 at 19:5

I still dislike the idea because it has to search in entire string for each of 50+ template placeholders and is still error prone. Please note that users of my application are the ones that supply both the data and the templates. – Shiller 18/3, 2009 at 19:7

As "Uncle" Bob Martin mentioned in a previous podacast with Joel and Jeff, pretty much anything you come up with is going to essentially be reproducing the big switch statement.

If you feel better implementing one of the solutions selected above, that's fine. It may make your code prettier, but under the covers, it's essentially equivalent.

The important thing is to ensure that there is only one instance of your big switch statement. Your switch statement or dictionary should determine which class handles this tag, and then subsequent determinations should be handled using polymorphism.

Weft answered 18/3, 2009 at 19:31 Comment(7)

This is not true (and is typical of the crap that Martin comes out with). To add to a switch I need to modify the code of the switch - I can add to the dictionary without modifying existing code at all. – Adolfo 18/3, 2009 at 19:38

Adding to the dictionary is still a code change, and one the compiler has less chance of picking up issues with.. it's not like the dictionary is being populated from data, it's going to be populated via code. Adding cases to a switch statement doesn't need to affect any existing cases...? – Deception 18/3, 2009 at 21:14

In theory, yes, the dictrionary could be populated from a config file or a database table, so it could be modified without a re-compile. In practice, if you are changing the mappings, it's probably because you have a new handler, so you're already doing a re-compile. – Weft 18/3, 2009 at 21:38

@john in practice yes too - you can use a plugin architecture, implemented with DLLs (or similar) that requires no access to the existing codebase at all. No recompile needed (or possible). – Adolfo 18/3, 2009 at 21:44

@Neil Butterworth: In a deep philosophical sense, you really are moving the switch statement. That move may change the form, extensibility, and other facets, but the essence of what happens is unchanged. However, many of the replacements are much more maintanable/extensible than the original. – Powwow 18/3, 2009 at 23:3

I'm totally with Neil on this. With a dictionary, you don't violate the Open/Closed Principle -- to add a new handler, just derive a new class from a base class, no changes or recompilation to the factory are required. As he said, this makes it possible to deliver plugins/DLLs after deployment. – Kuehnel 19/3, 2009 at 4:46

@Harper: To say that, at a very abstract level (e.g. the level of observable behaviour), switches are equivalent to dictionaries is correct, but misses the point. Improving extensibility while maintaining behaviourally-equivalent code is the whole point of good design. – Kuehnel 19/3, 2009 at 4:50

Recommended topics

Hot tags