views:

211

answers:

6

Hi,

I am porting some code from C to C++ and I found this code:

if(ErrorCode >= SOME_CONSTANT)
{
    Status = RETVAL_OK;

    switch ( ErrorCode )
    {
        default:
            Status = RETVAL_FAILED;
            break;
    }
}

This code generates a compilation warning:

warning C4065: switch statement contains 'default' but no 'case' labels

The question: Is there any purpose of the switch statement (that I failed to grasp) or is it simply crufty code?

That is, is there any reason (when compiling for ANSI C or C++) to not write it like this?

if(ErrorCode >= SOME_CONSTANT)
     Status = RETVAL_FAILED;

EDIT: To address all the questions that appeared:

The code was not meant to be expanded: It was the final release of a module that was delivered four years ago (it hasn't been touched since then, so I am inclined to believe it's cruft).

There were also no removed case statements as far as I could see (the same code construct was placed in three different places in code (the same switch/default inside an if checking on the error constant). If there were removed case statements, the code should have been refactored anyway when the cases were removed.

Thanks everyone.

+2  A: 

It is Crufty code.

Shaji
+2  A: 

The code as it stands is redundant and Status will always be equal to RETVAL_FAILED. Looks like the author had intended to provide additional logic but left that unfinished for some reason.

bjg
+2  A: 

Maybe the code was generated somehow, and the generator may depend on this syntax to extend it. But other than that, I do not see any reason why you would need this complex code.

Frank
+8  A: 

Two things I can think of: 1) the code was automatically generated 2) the original coder thought they might add different processing for error codes later, but never did. In either case, I can't see any reason not to change it to a simple if statement

Chris Card
The other possibility that I can think of is that there were originally some additional cases listed there that were inside of a `#ifdef .... #endif`, and the block was removed before you got the code (for instance, the conditional block was code for a different platform and multi-platform support was stripped out before you got the code). In any case, the original author must have never attempted to compile this code, so beware of other problems elsewhere.
bta
+2  A: 

The only advantage I can see to writing it this way is that if there were to be multiple possible error codes, this program would be easier to adapt to that by adding case statements.

In your position, I'd ask myself how likely a different error code would be in this project's future. If likely, keep this structure, otherwise ditch it.

psicopoo
A: 

If ErrorCode is an enumeration then probably you may want to check for the other items in the enum and use them in the switch statement.

Alternatively i think the original program contained another function call

ErrorCode = Func();

Now it makes sense to have a

switch(ErrorCode)
{
case enum1:
break;
case enum2:
break;
default:
break;
}

This kind of approach is followed when you have to do several tasks in a function and return value is common.

Praveen S