2

I had a HUGE switch/case statement, which I converted to use a function dispatch table using an enum list for the index.

My colleague (who is doing a code review on this change) agreed that it should be broken down into several functions, but there was a later change that required that some parameters that weren't used in all of the functions.

The code has not yet been finalized and his suggestion is that instead of a dispatch table, that I use a switch/case that calls the functions. His rationale is that because not all of the functions will use all of the parameters, that calling each individual function with slightly different signatures would be better than using a single function signature for all of the individual functions.

I'm not too adverse to this idea, but I'm not really convinced that this is really a step up either. The parameters are all POD types, and only the last two which are forwarded are not necessarily used. So there is no real performance issue, even if it were called a lot, which it is not (requires that the user press a button, making the dispatch time insignificant in comparison).

This is roughly the current layout that I migrated:

enum FindWhere_e : int
{
    FindInA = 0, // Find Where
    FindInD = 1,
    FindInC = 2,
    FindInE = 3,
    FindInH = 4,
    FindInI = 5,
    FindInJ = 6,
    FindInK = 7,
    FindInL = 8,
    FindInM = 9,
    FindInB = 10,
    FindInN = 11,
    FindInO = 12,
    FindInP = 13,
    FindInQ = 14,
    FindInF = 15,
    FindInR = 16,
    FindInG = 17,
};

BOOL Paragraph::DoSearch(IParaIdxC ipara, int ichar, FindReplaceData *frData, BOOL firstpara, BOOL lastpara, int *nchanges, CDocEnvironment& docEnv, bool allowRedisplay, Section *pSection, CTextWindow* textWindow)
{
    typedef decltype(&Paragraph::DoSearch) vtableElement_t;
    static vtableElement_t const jumpTable[] =
    {
        &Paragraph::DoSearch_FindInA,        // FindInA
        &Paragraph::DoSearch_FindIn_D_C_B_O, // FindInD
        &Paragraph::DoSearch_FindIn_D_C_B_O, // FindInC
        &Paragraph::DoSearch_FindInE,        // FindInE
        &Paragraph::DoSearch_FindInH,        // FindInH
        &Paragraph::DoSearch_FindInI,        // FindInI
        &Paragraph::DoSearch_FindInJ,        // FindInJ
        &Paragraph::DoSearch_FindIn_K_L,     // FindInK
        &Paragraph::DoSearch_FindIn_K_L,     // FindInL
        &Paragraph::DoSearch_FindInM,        // FindInM
        &Paragraph::DoSearch_FindIn_D_C_B_O, // FindInB
        &Paragraph::DoSearch_FindInN,        // FindInN
        &Paragraph::DoSearch_FindIn_D_C_B_O, // FindInO
        &Paragraph::DoSearch_FindInP,        // FindInP
        &Paragraph::DoSearch_FindInQ,        // FindInQ
        &Paragraph::DoSearch_FindInF,        // FindInF
        &Paragraph::DoSearch_FindInR,        // FindInR
        &Paragraph::DoSearch_FindInG,        // FindInG
    };
    // frData->Where is of type FindWhere_e
    if ((unsigned int)(frData->Where) < std::extent<decltype(jumpTable)>::value)
    {
        return (this->*jumpTable[frData->Where])(ipara, ichar, frData, firstpara, lastpara, nchanges, docEnv, allowRedisplay, pSection, textWindow);
    }
    ASSERT(false); // Not handling an unregistered find type in the dispatch table.
    return FALSE;
}

So, I'd like to know if there is any reason for using one over the other, or if it really doesn't matter from a design standpoint. I personally don't really find either clearer to read.

  • How many cases? – Robert Harvey Dec 8 '15 at 22:53
  • @RobertHarvey ??? The enum shows 18. But the dispatch table shows less as some are duplicated. – Adrian Dec 8 '15 at 22:55
  • Oh. That doesn't seem HUGE to me. A thousand would be HUGE. – Robert Harvey Dec 8 '15 at 22:57
  • @RobertHarvey, the original function was many pages long. Anything larger than a page to me is huge. Prolly around 1000 lines of code. – Adrian Dec 8 '15 at 22:58
  • 1
    I'm voting to close this question as off-topic because it is for codereview.stackexchange.com – BЈовић Dec 9 '15 at 9:45
2

Well, let's first look at the performance-argument:

  • Your dispatcher has the same arguments as the implementation-functions.
  • All the arguments are trivially copyable.
  • Your dispatcher must not do any cleanup after dispatching, and returns the called implementation-functions arguments verbatim.

Thus, your implementation should perform tail-call-optimization, if it doesn't simply inline them.
It is quite possible that the compiler is more inclined to inline the calls in the switch-statement than in your indexed-call, which might reverse who has the advantage.
As always, if you really care about it, measure!

Aside from that, even if you decide to allow for out-of-range arguments in production (as a way to future-proof??), consider putting the unexpected case in an if-block, as that often leads to better performance.

BOOL Paragraph::DoSearch(IParaIdxC ipara, int ichar, FindReplaceData *frData,
    BOOL firstpara, BOOL lastpara, int *nchanges, CDocEnvironment& docEnv,
    bool allowRedisplay, Section *pSection, CTextWindow* textWindow)
{
    static const decltype(&Paragraph::DoSearch) jumpTable[] = {
        ... omitted
    };
    if((unsigned)frData->Where >= std::extent<decltype(jumpTable)>()) {
        assert(0);
        return FALSE;
    }
    return (this->*jumpTable[frData->Where])(ipara, ichar, frData, firstpara,
        lastpara, nchanges, docEnv, allowRedisplay, pSection, textWindow);
}
  • Does it really matter if the jump table call is at the end of the function or not? – Adrian Dec 9 '15 at 16:52
  • The important part is that the compiler normally assumes a slight bias for the if-statement not being taken. As an aside, I would either call std::terminate() or not check in release-builds at all, unless those non-recognized values could be legit. – Deduplicator Dec 9 '15 at 17:01
1

From the moment that some functions have been introduced which accept a different number of parameters than the rest, you do not have a perfectly uniform situation in your hands anymore.

It is not a mortal sin to try to shoehorn a non-perfectly uniform situation into a perfectly uniform situation, but I have a slight aversion to doing that sort of thing: if there is a lack of uniformity, I do not like pretending that there is uniformity.

And the future usually proves that over time, the small lack of uniformity tends to become an ever increasing lack of uniformity, requiring more hacks and tricks to try to maintain the illusion of uniformity.

So, I would go with the switch case.

(I am not mentioning performance considerations, since you have already stated that performance is not an issue.)

Not the answer you're looking for? Browse other questions tagged or ask your own question.