tags:

views:

566

answers:

7
enum SQLErrorCode{
      OK = 0,
      PARTIAL_OK = 1,
      SOMEWHAT_OK = 2,
      NOT_OK = 3,
};

Code 1:

int error = getErrorCode();
if((error == SQLErrorCode.PARTIAL_OK) ||
  (error == SQLErrorCode.SOMEWHAT_OK) ||
  (error == SQLErrorCode.NOT_OK) ||
  (error < 0))
   callFunction1();
else
    callFunction2();

Code 2:

switch(error){
       case SQLErrorCode.PARTIAL_OK: 
                                    callFunction1();
                                    break;
        case SQLErrorCode.SOMEWHAT_OK:
                                    callFunction1();
                                    break;
        case SQLErrorCode.NOT_OK: 
                                    callFunction1();
                                    break;
        default:
                                    callFunction2();
                                    break;
}

Which method should I prefer. As far as performance is considered there should not be much difference. How to handle error < 0 condition in switch case.

EDIT: Joel's solution:

switch(error) {
     case SQLErrorCode.PARTIAL_OK: 
     case SQLErrorCode.SOMEWHAT_OK:
     case SQLErrorCode.NOT_OK: 
         callFunction1();
         break;
     case SQLErrorCode.OK:
         callFunction2();
         break;
     default:     // error < 0 is handled
         callFunction1();
         break;
}

Q. error < 0 is handled. If I have to handle other numbers in error which do not belong to any of the cases here including default.

+11  A: 

Without expressing preference for which is best, here is another possibility:

switch(error){
    case SQLErrorCode.PARTIAL_OK: 
    case SQLErrorCode.SOMEWHAT_OK:
    case SQLErrorCode.NOT_OK: 
                                callFunction1();
                                break;
    default:
                                callFunction2();
                                break;
}
Adam Batkin
Much better than the original switch function.
David Thornley
Oops!! I missed that :| My bad!
Devil Jin
Hey, we're all allowed to screw up once... but next time... you're done for :)
Matthew Whited
This would be my preference. Related question: http://stackoverflow.com/questions/188461/switch-statement-fallthrough-should-it-be-allowed
Fred Larson
Ok, this one is "elegant" but I wouldn't use this method. Why, you may ask? This one can be very dangerous when you suddenly wants to handle SOMEWHAT_OK with another function. It is easy to break the whole logic here. I didn't down this answer, but David's comment is aligned with what I'm thinking.
wtaniguchi
-1: Despite all the happy up votes, this doesn't answer the OP's question about error valuess outside the known range.
Joel Potter
Sure it does. That's what the `default` is there for (well, the code may not work exactly the way the poster wants it to, but the example is - by design - vague). This was meant to just illustrate another way of laying out the code since when I wrote it, everything from "EDIT: Joel's solution" to the bottom, wasn't there.
Adam Batkin
+1  A: 

I haven't touched C in awhile, but doesn't it have fall through? So you could write the second chuck as such...

switch(error){
   case SQLErrorCode.PARTIAL_OK: 
    case SQLErrorCode.SOMEWHAT_OK:
    case SQLErrorCode.NOT_OK: 
                                callFunction1();
                                break;
    default:
                                callFunction2();
                                break;

}

Hawker
+2  A: 

Why not...

switch(error) {
    case SQLErrorCode.PARTIAL_OK: 
    case SQLErrorCode.SOMEWHAT_OK:
    case SQLErrorCode.NOT_OK: 
         callFunction1();
         break;
    case SQLErrorCode.OK:
         callFunction2();
         break;
    default:
         if (error < 0)
              callFunction1();
         else
              callFunction2();
         break;
}

Easier to write than your switch, and easier to read than your if. Yet it still handles error < 0.

EDIT:

Richard brings up a good point. I've edited to handle positive and negative errors outside the known range.

Joel Potter
I voted for this one although I prefer to have break aligned with the word case myself.
KPexEA
What happens when 'error' is a positive value other than one of those listed in the enumeration? for example 10? In the original code, "callFunction2" would be called, however, in your code "callFunction1" is called. This behaviour may not be "wrong", however, it is different to the original code.
Richard Corden
+1  A: 

Whenever you have multiple ways to get the same effect, you're causing confusion. Since, in the switch version, you've got different cases for SqlErrorCode.PARTIAL_OK and SqlErrorCode.SOMEWHAT_OK, the implication is that they have different processing. It takes some study to see what is happening (and it isn't completely compatible with your if-statement handling, which probably means it confused you).

In this case, I'd use an if statement, since the idea is to use one function or another.

David Thornley
You are very true here. I am confused here about which one should I prefer.
Devil Jin
+2  A: 

Assuming getErrorCode() returns one of your enumerated values or something less than 0, how about

int error = getErrorCode();
if (error == SQLErrorCode.OK)
  callFunction2(); // Good path
else
  callFunction1(); // Error / not good enough path

Obviously, if your code needs to callFunction2() on error > 3, then this doesn't work.

Novelocrat
I was about to suggest the same thing.
Rob K
+2  A: 

You could also write a function that will determine what an OK error or not OK error code is:

bool isOK(int code)
{
  return code == SQLErrorCode.OK;
}

and your code could become:

if (isOk(getErrorCode()))
{
  callFunction2;
}
else
{
  callFunction1;
}
JRL
+1: This is the only answer that seems to have grasped that the switch statements are overkill for saying " != OK".
Richard Corden
+5  A: 

Not that it matters much for such a small number of cases but switch is in fact faster for integers: it can be, and often is, implemented as a jump table instead of a series of conditional checks.

As a comparison up the number of different cases to 10:

enum SQLErrorCode{
    CODE0 = 0,
    CODE1 = 1,
    CODE2 = 2,
    CODE3 = 3,
    CODE4 = 4,
    CODE5 = 5,
    CODE6 = 6,
    CODE7 = 7,
    CODE8 = 8,
    CODE9 = 9
};

enum SQLErrorCode getErrorCode();

void run()
{
    int error = getErrorCode();
#ifdef CASE1
    if((error == CODE0) ||       
       (error == CODE1) ||
       (error == CODE2) ||
       (error == CODE3) ||
       (error == CODE4) ||
       (error == CODE5) ||
       (error == CODE6) ||
       (error == CODE7) ||
       (error == CODE8) ||
       (error == CODE9) ||
       (error < 0))
        callFunction1();
    else
        callFunction2();
#endif
#ifdef CASE2
    switch(error)
    {
        case CODE0:
            callFunction1();
            break;
    case CODE1:
        callFunction1();
        break;
    case CODE2:
        callFunction1();
        break;
    case CODE3:
        callFunction1();
        break;
    case CODE4:
        callFunction1();
        break;
    case CODE5:
        callFunction1();
        break;
    case CODE6:
        callFunction1();
        break;
    case CODE7:
        callFunction1();
        break;
    case CODE8:
        callFunction1();
        break;
    case CODE9:
        callFunction1();
        break;
    default:
        callFunction2();
        break;
}
#endif

}

Now look at the assembly generated by doing the first case vs. the second, when built on Linux using GCC.

If you look at the assembly you ll see a significant difference (for larger statements): the series of ||s (or if/else if you do it that way) is a series of branches taken one at a time. The switch turns into a big table: it takes up more code but can mean that it can be handled in one jump.

(Incidentally, we are talking about C here right? Not C#? The code you have won't compile: in C enumerators don't use the enumeration name as a prefix. So it's PARTIAL_OK without the SQLErrorCode.)

Code 1: cc -DCASE1 -s switch.s switch.c

        .file   "1241256.c"
        .text
.globl run
        .type   run, @function
run:
        pushl   %ebp
        movl    %esp, %ebp
        subl    $24, %esp
        call    getErrorCode
        movl    %eax, -4(%ebp)
        cmpl    $0, -4(%ebp)
        je      .L2
        cmpl    $1, -4(%ebp)
        je      .L2
        cmpl    $2, -4(%ebp)
        je      .L2
        cmpl    $3, -4(%ebp)
        je      .L2
        cmpl    $4, -4(%ebp)
        je      .L2
        cmpl    $5, -4(%ebp)
        je      .L2
        cmpl    $6, -4(%ebp)
        je      .L2
        cmpl    $7, -4(%ebp)
        je      .L2
        cmpl    $8, -4(%ebp)
        je      .L2
        cmpl    $9, -4(%ebp)
        je      .L2
        cmpl    $0, -4(%ebp)
        jns     .L13
.L2:
        call    callFunction1
        jmp     .L15
.L13:
        call    callFunction2
.L15:
        leave
        ret
        .size   run, .-run
        .ident  "GCC: (GNU) 4.2.4 (Ubuntu 4.2.4-1ubuntu4)"
        .section        .note.GNU-stack,"",@progbits

Code 2: cc -DCASE2 -s switch.s switch.c

        .text
.globl run
        .type   run, @function
run:
        pushl   %ebp
        movl    %esp, %ebp
        subl    $24, %esp
        call    getErrorCode
        movl    %eax, -4(%ebp)
        cmpl    $9, -4(%ebp)
        ja      .L2
        movl    -4(%ebp), %eax
        sall    $2, %eax
        movl    .L13(%eax), %eax
        jmp     *%eax
        .section        .rodata
        .align 4
        .align 4
.L13:
        .long   .L3
        .long   .L4
        .long   .L5
        .long   .L6
        .long   .L7
        .long   .L8
        .long   .L9
        .long   .L10
        .long   .L11
        .long   .L12
        .text
.L3:
        call    callFunction1
        jmp     .L15
.L4:
        call    callFunction1
        jmp     .L15
.L5:
        call    callFunction1
        jmp     .L15
.L6:
        call    callFunction1
        jmp     .L15
.L7:
        call    callFunction1
        jmp     .L15
.L8:
        call    callFunction1
        jmp     .L15
.L9:
        call    callFunction1
        jmp     .L15
.L10:
        call    callFunction1
        jmp     .L15
.L11:
        call    callFunction1
        jmp     .L15
.L12:
        call    callFunction1
        jmp     .L15
.L2:
        call    callFunction2
.L15:
        leave
        ret
        .size   run, .-run
        .ident  "GCC: (GNU) 4.2.4 (Ubuntu 4.2.4-1ubuntu4)"
        .section        .note.GNU-stack,"",@progbits
quark
that is exactly what I was looking for!
Devil Jin
Good. I like reading that :)
quark
Wow, that's -O0?!
LiraNuna
Yes, that's with the default (the command lines shown are the one I used).
quark