views:

200

answers:

7

I have certain code that I want to optimize. It looks like this:

function abc( string format ) {
  if (format == "a") { // this is a string, I shouldn't have used single quote, sorry for the confusion
    classx::a t;
    doit(t);
  }
  if (format == "b"){
    classx::b t;
    doit(t);
  }
  if (format == "c"){
    classx::c t;
    doit(t) 
  }
  if (format == "d"){
    classx::d t; 
    doit(t);
  }
}

Currently there is many doit() function with different type

function doit( classx:a ) {
   different code for a
}

function doit( classx:b ) {
   different code for b
}

...etc

As you can see, a lot of code is replicated. However I can't figure out how to reduce the words. Note that : doit(x) has overloaded by different type. a,b,c,d class is derived from a class named "X".

I may create a pointer type classx::X :

classx::X *t;
if (format == "a") t = new classx::a
if (format == "b") t = new classx::b
if (format == "c") t = new classx::c
if (format == "d") t = new classx::d
doit(*t)

but then still need to write a doit() for type classx::X with a bunch of "if then" and cast to the correct type... as C++ can't auto-detect and cast to correct type.

I wonder if there is a faster/smarter way to do this. Thanks in advance.

+1  A: 

Put the format/constructor pairs into a dictionary. The key is that format string, the value is a function pointer to a static factory method that is essentially just a thin wrapper over the constructor. Besides being easier to maintain, it'll do a hash lookup or binary search, depending on the sort of dictionary/map you use.

Steven Sudit
While that would accomplish what he's asking to do, it introduces complexity where there need not be any.
SilverSun
Putting aside your strategic downvote, the reason this is a better idea is that it scales nicely and is very clean. Each class can register itself with the factory during start-up, and this can be entirely dynamic. It's a best practice.
Steven Sudit
Self-registration doesn't scale, because it doesn't play well with libraries. The linker does not arrange to call all global (static) constructors in a library, only those in the same compilation unit as code that is directly referenced.
Ben Voigt
@Ben: That's a good point, but there are various ways around that. The very worst case is that each classes RegisterMySelf function has to be called at some point prior to the factory being used, which is still better than having a single block of code that has all the children hardcoded.
Steven Sudit
Sorry I can't get the idea. Could you give me a sample code ? what is the return of the "function pointer to a static factory"
iKid
Basically, you'd do a dictionary lookup, passing in the string and getting a pointer to a function that returns a new instance of some child class. It would be a single line, something like: `classx::X *t = lookup[format]();`
Steven Sudit
this way I still need to define a doIt( class::X ) and dispatch it to doIt(class::a) , doit(class::b) later, as doIt( class::a ) won't be able to detect a class::X type.
iKid
The best way to do something different depending on which subtype an instance is would be to leverage polymorphism. In other words, `doIt` would be a virtual method in classx::X, which the child classes would all override. This avoids any need for switch statements.
Steven Sudit
Since nobody mentioned it so far: This is an application of the Prototype pattern of the GoF "Design Patterns".
Peter G.
@Peter: This is a fine point that I'm not particularly hung up on, but wouldn't a Prototype pattern require that the values in the dictionary be instances of each type so that they can be cloned off? What I've presented is also a creational pattern, but I think it's more generic; perhaps it qualifies as a Factory Method.
Steven Sudit
@Steven Agreed, called you solution Prototype was somewhat sloppy. Still, Prototype would be my inspiration for your solution.
Peter G.
@Steven: Lookup of function pointer and their invocation can't sensibly be done in one statement - you have to check wether there actually is a function present for the key.
Georg Fritzsche
@Georg: Maybe. One reasonable alternative is to throw an exception if it's not found. Another is to use a lookup method that allows for defaults, so that the base class can be returned in that case. Even if we have to code it so that we get the value into a local, check it and handle special cases, it's still a fixed, small number of lines, as opposed to one code block per subtype.
Steven Sudit
@Peter: I'd agree that it's functionally similar to a Prototype. My concern here is more with false advertising than in finding the perfect term. In particular, I am not particularly interested in whether I even officially qualify as some "official" design.
Steven Sudit
I would agree that the best solution is to use a virtual function and put the code of doIt() in derived class... However the refactoring is quite costly as it affects other code. Maybe I just bear with it for a while.
iKid
@Steven: Ok, i was thinking STL containers for which that doesn't apply out-of-the-box. Sure its better than multiple code-blocks, i didn't say otherwise.
Georg Fritzsche
@Georg: You're right about STL.
Steven Sudit
@iKid: Hey, you're the one closest to the code, so you get to make the judgement call here. Perhaps you could deal with the current architecture for now and work towards refactoring it when you have the opportunity.
Steven Sudit
+1  A: 

It will be faster if you use else if after the first if so that it doesn't keep testing after finding a match. This is more compact and simpler to read as well...

function abc(string format) {
    if (format == 'a')
        doit(classx::a());
    else if (format == 'b')
        doit(classx::b());
    else if (format == 'c')
        doit(classx::c())
    else if (format == 'd')
        doit(classx::d());
}
SilverSun
Thanks for the strategic downvote, but your solution is actually a step in the wrong direction. If they were to do this entirely without a data structure, the right answer would be switch/case. Not only does it avoid unnecessary tests after finding the match, but some compilers do a good job optimizing it further with lookup tables and even perfect hashes. Having explained why your answer is bad, I see no reason add injury by downvoting you.
Steven Sudit
I changed the list of `if` statements to use `else if` to avoid exactly that. A good optimizing compiler would have optimized that anyway, just saying.
SilverSun
Also a switch statement would only work if the parameter were a `char` and not a `string`.
SilverSun
Last I checked, single quotes meant character, while double quotes meant string. The original sample contradicts itself by using single quotes but claiming the parameter is a string. Since the values are single characters, I think it's safe to assume that the parameter is either a char, or it's a single-character string. Either way, switch/case works. A good optimizing compiler can do more with switch/case than if/then/else.
Steven Sudit
A switch/case will not work with a string. Assuming he meant to use a 'char' is valid and to your points relating I agree.
SilverSun
It'll work fine on `format[0]`.
Steven Sudit
If he actually intended a multi-character string, then this leaves us with a strong case for the dictionary-based class factory.
Steven Sudit
just to clarify... I intentionally put it as string because in my actual code the format is more complex and can't use switch
iKid
Don't forget that if you know a significant amount about your domain AND if it has a big variation in the frequency of cases, a sensible ordering of if else if can be more efficient than switch. However, I agree that in most situations using a char value in a switch would be preferable.
Andy Dent
A: 

Can you post what you have in mind for the doit() function, are there different overrides for the different types or is there a single override that takes a base class pointer/ref?

DevinEllingson
1) This belongs as a comment, not an answer. 2) I'm not sure that it matters, although I'm also curious.
Steven Sudit
I have updated my question
iKid
Thanks @Steven Sudit, unfortuntately you can't comment until you have at least rep 50 points, not sure why, this seems like a strange rule. But I have more than 50 points now so it isn't an issue.
DevinEllingson
If the doit function doesn't have any common code then the overrides are not really related except by name, thus your first set of code is probably the simplest and most readable way.
DevinEllingson
@Devin: Ah, ok.
Steven Sudit
A: 

Here's an approach using macros that assumes that format is really a string. The single quotes you are using in the original (Javascript?) code are for characters.

I can't work out anything remotely as compact using templates, yes, sometimes macros are still useful!

#define FORMATTER(ltr) \
    if (format == #ltr) { \
    classx::##ltr t; \
    doit(t); \
  }

#define ELSEFORMATTER(ltr) else FORMATTER(ltr)

void abc( std::string format ) {
    FORMATTER(a)
    ELSEFORMATTER(b)
    ELSEFORMATTER(c)
    ELSEFORMATTER(d)
}
Andy Dent
are you sure that is correctly to generate single quotes?
aaa
how about double quotes ?
iKid
I have the same reservations about this as I do about templates, plus additional ones from the obscuring effect of the precompiler.
Steven Sudit
A: 

using boost preprocessor

#define MACRO(r, data, elem)                     \
if (format == '(elem)')  doit(classx::(elem)()); \
else

BOOST_PP_SEQ_FOR_EACH(MACRO, _, (a)(b)...) {
... // else condition
}

I am not sure how to put macro inside '' however: http://stackoverflow.com/questions/2072532/how-to-single-quote-an-argument-in-a-macro

aaa
I have the same reservations about this as I do about templates, plus additional ones from the obscuring effect of the precompiler.
Steven Sudit
+3  A: 

One possible approach that reduces the repetition to adding new entries to a function map:

template<class T> void innerAbc() {
    T t;
    doit(t);
}

typedef std::map<std::string, void (*)()> FuncMap;

FuncMap initHandlers() {
    FuncMap m;
    m["a"] = &innerAbc<classx::a>;
    // ... extend here
    return m;
}   

void abc(const std::string& format) {
    static const FuncMap handlers = initHandlers();
    FuncMap::const_iterator it = handlers.find(format);
    if (it != handlers.end()) 
        it->second();
}
Georg Fritzsche
Using function pointers instead of predicates is very smart here, since you don't have to worry about memory management! Also, with the advent of lambda functions, it should become easier and easier (if the function was a tad more complicated).
Matthieu M.
My concern here is that you've reinvented the vtable.
Steven Sudit
@Steven: This doesn't replace a vtable, the overloads for `doit()` might go there. But then it was already suggested that it *might* work better as a virtual member function.
Georg Fritzsche
My point here is that anything that maps a type to a method looks suspiciously like a vtable already, so your solution is in some ways reimplementing what is already available. On the other hand, a factory driven by map is doing some a vtable cannot do.
Steven Sudit
@Steven: It maps to a static type, which a factory or similar would have to do anyway before you could act on some derived class it created. If however we already have the static type, virtual functions don't even need to come into play - wether `doit()` is a member function or not.
Georg Fritzsche
The idea of mapping a type to a function is at the heart of vtables, which is why I see a connection here.
Steven Sudit
+1  A: 

A simple template approach gets rid of much of the duplicated code. If you want to avoid having a series of 'if' statements, you can use a map, or a sorted vector with binary search.

template<typename T> void forward_doit()
{
    T t;
    doit(t);
}

void func(string const& s)
{
    if (s == "a") return forward_doit<Classx::a>();
    if (s == "b") return forward_doit<Classx::b>();
    if (s == "c") return forward_doit<Classx::c>();
    // ...
}
janm
I think using template is a quite elegant approach to my problem. This answer is same as from Georg Fritzsche :) but easier to get the idea.
iKid
This is a good way to shorten the amount of code, but it doesn't solve the deeper problems. Namely, this is still a linear search for the right type, and it requires `func` to know about all children of the base. Not maintainable.
Steven Sudit
@Steven: Whether not it is maintainable depends on factors not stated in the question. We don't know whether a linear search is worse or better because we don't know how many items there are and whether some are called more often that others. While func() needs to know about all the implementation types, the mapping between the key and the type must be somewhere and we don't know where is appropriate in this case. The point of my answer is to describe how to avoid the duplicated code; I think the simplicity of a single line per case is a good start. Why add complexity unless necessary?
janm
If only it weren't necessary! You have *some* point regarding linear probably being fast enough for a small number, but where I cannot agree is the idea that a naturally expandable system -- subclassing -- should be limited by having a single function that needs to know about all subclasses. A dictionary-based approach is not only faster for lookups when the number of subclasses increases, it also allows the addition of entries from multiple locations at different times. This is a big deal and I'd argue that it's worth a little bit of complexity.
Steven Sudit
@Steven: It is only worth paying the complexity price if you get something out of it. I agree that in many cases a dictionary based approach is good, but I don't know that this is one of those cases. As I said, use a map or sorted vector if appropriate. As for where the addition entries occurs, consider that func() might implement a comms protocol and dispatch on a protocol element, and call doit() when appropriate. The classes called have no knowledge of the protocol and should not have that knowledge, and so could not self register.
janm
@janm: What you get out of it is flexibility in precisely the area that needs it. Whether subclasses self-register or are registered by other code, a dictionary approach means there doesn't have to be a single point of omniscience.
Steven Sudit