2

I found a piece of code on the internet that has a really simple objective, yet it uses an ugly approach. Supposedly, the author is using a switch case to determine if a few (non-contiguous) values of a previously-defined Enum belong in their scope. If it does, the function returns true and that's that. Else, it returns false.

It practically looks like this:

switch(value) {
case ONE:
case TWO:
/* many similar lines later */
case TWENTY:
case TWENTY_FIVE:
/* an afternoon later */
case ONE_HUNDRED:
    return true;
default:
    return false;
}

Their use of a switch case is justified with having an instant lookup thanks to a jump table generated by the compiler (even though a jump table doesn't necessarily mean instant lookup from what I've gathered). Even so, this generates countless needless lines of code.

I've read about function inlining and using an array of function pointers, but I don't know how to use that in such a specific case.

How do I avoid writing many lines of case X: with such a simple case (no pun intended) as this?

  • 2
    Utterly long `switch` and/or `if` statements are usually a sign of poor usage of OOP – hgiesel Feb 29 '16 at 18:21
  • 2
    @henrikgiesel, where did OOP come from? – SergeyA Feb 29 '16 at 18:22
  • 1
    What exactly is your question? I don't get it. – Christian Hackl Feb 29 '16 at 18:24
  • 4
    `if (value > ONE && value < ONE_HUNDRED)`? – SergeyA Feb 29 '16 at 18:24
  • SergeyA is right. why you are talking about OOP? @henrikgiesel – Md. Shohan Hossain Feb 29 '16 at 18:24
  • 2
    What do you expect to gain by using an array of function pointers? If an array of function pointers was faster than a switch, compiler developers would start implementing switches with functions pointers... – fredoverflow Feb 29 '16 at 18:24
  • @ChristianHackl How to avoid writing many lines of code with such a simple objective and with appropriate data structures –  Feb 29 '16 at 18:25
  • Are the 100 cases contiguous numbers? – fredoverflow Feb 29 '16 at 18:25
  • Initializing a big array is also lot of boilerplate... if `ONE` .. `HUNDRED` are contiguous, just use `return ONE <= value && value <= ONE_HUNDRER`). – Jarod42 Feb 29 '16 at 18:25
  • Are the enum values contiguous? If so, SergeyA has the right idea. I know some people get queasy about `>` and `<` comparisons with enums, but if you control the enum definition, I see no problem. – Kyle A Feb 29 '16 at 18:26
  • @SergeyA The enum values that belong in this switch case have values that do not follow the order of the Enum declaration; they're a select few batches from the original Enum declaration. –  Feb 29 '16 at 18:27
  • Then the switch probably already is the best solution. – fredoverflow Feb 29 '16 at 18:28
  • This might be one of the cases where the use of a macro would be okay. If you need to guarantee that code is inlined then you can use a macro as it is a direct text replacement. This will shorten the code and give you the guarantee you need that there isn't a function call. – NathanOliver Feb 29 '16 at 18:29
  • Can you compare as SergeyA pointed in a few split areas? If not, any method will return that skiped numbers are valid within that enum. Switch case is probably your best option. – Rodrigo Guiotti Feb 29 '16 at 18:31
  • @fredoverflow They are not contiguous numbers, unfortunately –  Feb 29 '16 at 18:32
  • @RodrigoGuiotti Could try that, although it's a bit specific; but what can you do with such an odd code. –  Feb 29 '16 at 18:33

3 Answers3

8

Computing a boolean value efficiently based on some bound integral number is a job for bit twiddling:

const unsigned long long ps[2] = {0x28208a20a08a28ac, 0x800228a202088288};

bool is_prime(unsigned x)
{
    return (x < 128) && ((ps[x >> 6] >> (x & 63)) & 1);
}

If you look at the binary representation of the numbers stored in the array, a 1 bit signifies a prime number, and a 0 bit signifies a compound number:

   2    8    2    0    8    a    2    0    a    0    8    a    2    8    a    c
0010 1000 0010 0000 1000 1010 0010 0000 1010 0000 1000 1010 0010 1000 1010 1100
    59             47     41           31        23     17      11      5   2
 61        53           43     37        29           19     13       7    3

To scale this to more than 128 numbers, simply increase the array size and patch the < comparison in is_prime. The constants 6 and 63 stem from the amount of bits in an unsigned long long.

fredoverflow
  • 256,549
  • 94
  • 388
  • 662
2

Avoid a repeating and simple switch case in C/C++?

Yes, do avoid this.. You should normally work at the highest level of abstraction possible at any given domain/context, whether this is compile-time polymorphism (e.g. using templates), object-oriented programming or, simpler control structures.. First come correctness, code clarity and efficiency and then comes optimisation (and only after proper measurement)

In this particular case you could simply do this:

return (value < 100);

I don't see why a 100+ lines switch statement is better.. even if it's faster (which is just an assumption in the absence of actual measurements) it'll only be slightly faster anyway so does it deserve all this fuss?? No, or at least no in most real-life scenarios.. If the application is that critical, optimising such code is better done in assembly language anyway..

As far as function inlining and arrays of function pointers - I'm not sure I understand what the questions is, but nevertheless, if you don't understand how to use such features for optimisation, don't use them.. let the compiler optimise your code instead.

Marinos K
  • 1,779
  • 16
  • 39
  • 1
    Not being a C/C++ expert, why would a `switch` statement be faster? From my understanding, it has to compare the `value` 100 times, while `if` does it only once. – sjaustirni Feb 29 '16 at 18:35
  • Taking into account that not all enum values are present in this list (a.k.a. they're not contiguous), I could use, as @RodrigoGuiotti suggested, comparing the ends of each "bunch" of enum values. –  Feb 29 '16 at 18:42
0

Since you have an odd enum, skipping values, you can invert your solution. But you will need to declare skipped numbers (if it is skipping only a few).

if( value > ONE_HUNDRED || value < ONE )
{
    return false;
}
else
{
    switch(value)
    {
         case SKIPPED_FIRST:
         case SKIPPED_SECOND:
         {
             return false;
         }
         break;
         default:
         {
             return true;
         }
    }
}