Replace giant switch statement with what?

Question

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?

score 26 · Accepted Answer · answered Mar 18 '09 at 18:41

26

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

answered Mar 18 '09 at 18:41

1

Especially good if your dictionary can use a O(1) lookup like hashing. – Paul Tomblin Mar 18 '09 at 18:43
Great answer. A tad on the lean side, but definitely a good way to go. : ) – e.James Mar 18 '09 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 – Mar 18 '09 at 18:45
3

@ejames I've never understood why long answers are considered better than short ones – Mar 18 '09 at 18:46
What should I use for second parameter to map? map – Milan Babuškov Mar 18 '09 at 18:47
Simplest would be a map. – Eclipse Mar 18 '09 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... – Mar 18 '09 at 18:48
Otherwise, you can go with maybe map 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. – Eclipse Mar 18 '09 at 18:50
1

@Neil Butterworth Short answers with helpful comments are really just long answers that require more clicking... :) – Andy Mikula Mar 18 '09 at 18:52
@Josh: Good idea, but that type needs to be map -- i.e. you must use pointers to the base class. Otherwise you will get object slicing instead of polymorphic behaviour. – j_random_hacker Mar 19 '09 at 04:27
And where is the compile time checking? – user73993 Mar 23 '09 at 07:55

Steven Huwig · Answer 2 · 2009-03-18T21:29:50.447

5

You want replace conditional with polymorphism. Roughly:

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

where doStuff is overloaded.

edited Mar 18 '09 at 21:29

answered Mar 18 '09 at 18:43

Steven Huwig

20,015
9
55
79

3

Of course, the switch is just being moved somewhere else (the factory), which is where it should be. – Ed S. Mar 18 '09 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. – Steven Huwig Mar 18 '09 at 18:47
Actually, that template IS the configuration file. Users can alter it themselves. – Milan Babuškov Mar 18 '09 at 18:59
I meant "configure the mapping of names to behaviors" (listed above as myType). Then there will be no switch involved. – Steven Huwig Mar 18 '09 at 19:07
Ummmm... would that be "obj.doStuff()"? – Dan Mar 18 '09 at 20:50

Dave Swersky · Answer 3 · 2009-03-18T18:49:35.827

3

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.

edited Mar 18 '09 at 18:49

answered Mar 18 '09 at 18:44

Dave Swersky

34,502
9
78
118

I think it's pretty optimistic of you to think that HTML templates will be valid XML. :) – Steven Huwig Mar 18 '09 at 18:46

score 3 · Answer 4 · answered Mar 18 '09 at 19:01

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

+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. — j_random_hacker, Mar 19 '09 at 04:31

score 2 · Answer 5 · answered Mar 18 '09 at 18:44

2

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());

answered Mar 18 '09 at 18:44

Gordon Bell

13,337
3
45
64

performance hit, but probably worth it for code simplicity if absolute efficiency is not absolutely needed. +1 – Brian R. Bondy Mar 18 '09 at 18:48
1

If "titleValue" variable contains the string "%color%" it would not work properly. – Milan Babuškov Mar 18 '09 at 18:49
ya it is less safe as well, but more simple still :) – Brian R. Bondy Mar 18 '09 at 18:53
Yeah, probably better to make the tokens slightly more complex, like "[%color%]". – Gordon Bell Mar 18 '09 at 19:05
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. – Milan Babuškov Mar 18 '09 at 19:07

score 2 · Answer 6 · answered Mar 18 '09 at 19:31

2

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.

answered Mar 18 '09 at 19:31

JohnMcG

8,709
6
42
49

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. – Mar 18 '09 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...? – Bittercoder Mar 18 '09 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. – JohnMcG Mar 18 '09 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). – Mar 18 '09 at 21:44
2

@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. – Harper Shelby Mar 18 '09 at 23:03
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. – j_random_hacker Mar 19 '09 at 04: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. – j_random_hacker Mar 19 '09 at 04:50

Replace giant switch statement with what?

6 Answers6

Linked