tags:

views:

1215

answers:

6
+13  Q: 

Replace giant switch statement with what?

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?

+24  A: 

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

anon  2009-03-18 18:41:46
+1 For clarification, in C++ these are called std::map.
Eclipse  2009-03-18 18:43:24
Especially good if your dictionary can use a O(1) lookup like hashing.
Paul Tomblin  2009-03-18 18:43:58
Great answer. A tad on the lean side, but definitely a good way to go. : )
e.James  2009-03-18 18:44:22
@paul I don't think the the performance is often an issue here - the clarity of the resulting code is more important
anon  2009-03-18 18:45:42
@ejames I've never understood why long answers are considered better than short ones
anon  2009-03-18 18:46:34
What should I use for second parameter to map? map<string, what?>
Milan Babuškov  2009-03-18 18:47:31
Simplest would be a map<string, string>.
Eclipse  2009-03-18 18:48:37
@milan - up to you - could be an instance of a class, a function pointer, a class on which you call a sttaic method...
anon  2009-03-18 18:48:47
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.
Eclipse  2009-03-18 18:50:34
@Neil Butterworth Short answers with helpful comments are really just long answers that require more clicking... :)
Andy Mikula  2009-03-18 18:52:40
@andy point taken - I'll try to be less gnomic.
anon  2009-03-18 18:57:03
@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.
j_random_hacker  2009-03-19 04:27:06
And where is the compile time checking?
 2009-03-23 07:55:03
+4  A: 

You want replace conditional with polymorphism. Roughly:

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

where doStuff is overloaded.

Steven Huwig  2009-03-18 18:43:55
Of course, the switch is just being moved somewhere else (the factory), which is where it should be.
Ed Swangren  2009-03-18 18:45:00
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  2009-03-18 18:47:20
Actually, that template IS the configuration file. Users can alter it themselves.
Milan Babuškov  2009-03-18 18:59:07
I meant "configure the mapping of names to behaviors" (listed above as myType). Then there will be no switch involved.
Steven Huwig  2009-03-18 19:07:58
Ummmm... would that be "obj.doStuff()"?
Dan  2009-03-18 20:50:16
why yes, yes it would be. thanks.
Steven Huwig  2009-03-18 21:29:40
+3  A: 

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.

Dave Swersky  2009-03-18 18:44:31
I think it's pretty optimistic of you to think that HTML templates will be valid XML. :)
Steven Huwig  2009-03-18 18:46:18
+2  A: 

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());
Gordon Bell  2009-03-18 18:44:45
performance hit, but probably worth it for code simplicity if absolute efficiency is not absolutely needed. +1
Brian R. Bondy  2009-03-18 18:48:26
If "titleValue" variable contains the string "%color%" it would not work properly.
Milan Babuškov  2009-03-18 18:49:14
ya it is less safe as well, but more simple still :)
Brian R. Bondy  2009-03-18 18:53:12
Yeah, probably better to make the tokens slightly more complex, like "[%color%]".
Gordon Bell  2009-03-18 19:05:10
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  2009-03-18 19:07:31
+1 for your 1st comment Milan.
j_random_hacker  2009-03-19 04:29:20
+3  A: 

i'll combine 3 ideas:

  1. (from Steven Hugig): use a factory method that gets you a different class for each selector.
  2. (from Neil Butterworth): inside the factory, use a dictionary so you get rid of the big switch(){}.
  3. (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
Javier  2009-03-18 19:01:27
Research guy..aren't you
Warrior  2009-03-18 19:20:59
+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  2009-03-19 04:31:25
+1  A: 

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.

JohnMcG  2009-03-18 19:31:17
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.
anon  2009-03-18 19:38:28
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  2009-03-18 21:14:43
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  2009-03-18 21:38:02
@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).
anon  2009-03-18 21:44:46
@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  2009-03-18 23:03:25
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  2009-03-19 04:46:06
@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  2009-03-19 04:50:58

related questions

Of Memory Management, Heap Corruption, and C++
How do I make a GUI?
Alpha blending sprites in Nintendo DS Homebrew
Thread safe lazy contruction of a singleton in C++
Interview Programming Questions - In house Exam
Link issues (VC6)
What are the barriers to understanding pointers and what can be done to overcome them?
Why are professors or schools picking Java over C++ to teach to students?
What is the best way to create a sparse array in C++
C/C++ library for reading MIDI signals from a USB MIDI device
How do you pack a visual studio c++ project for release?
How to set up unit testing for Visual Studio C++
How do I configure and communicate with a serial port?
Lightweight IDE for Linux
Mapping Stream data to data structures in C#
CPU throttling in C++
Asynchronous multi-direction server-client communication over the same open socket?
Exceptions in C++
Heap corruption under Win32; how to locate?
Build for Windows NT 4.0 using Visual Studio 2005?
C++: Should I use nested classes in this case?
BerkeleyDB Concurrency
GTK implementation of MessageBox
Is gettimeofday() guaranteed to be of microsecond resolution?
How to use the C socket API in C++ on z/OS