views:

257

answers:

8

Hi All, I have an insert query that returns an int. Based on that int I may wish to throw an exception. Is this appropriate to do within a switch statement?

 switch (result)
        {

            case D_USER_NOT_FOUND:
                throw new ClientException(string.Format("D User Name: {0} , was not found.", dTbx.Text));
            case C_USER_NOT_FOUND:
                throw new ClientException(string.Format("C User Name: {0} , was not found.", cTbx.Text));
            case D_USER_ALREADY_MAPPED:
                throw new ClientException(string.Format("D User Name: {0} , is already mapped.", dTbx.Text));
            case C_USER_ALREADY_MAPPED:
                throw new ClientException(string.Format("C User Name: {0} , is already mapped.", cTbx.Text));
            default:

                break;
        }

I normally add break statements to switches but they will not be hit. Is this a bad design? Please share any opinions/suggestions with me.

Thanks, ~ck in San Diego

+3  A: 

I don't really agree with your variable naming conventions ;), but I don't see why what you did would be inappropriate. It seems like a fairly elegant way of translating an error from one medium to another.

I'm assuming that your "insert query" is some form of stored proc.

JustLoren
Yes sir you are correct. Thanks.
Hcabnettek
A: 

I think this is OK. It seems like you are mapping an return code to an exception, using a switch statement. As long as there are not too many cases, this is not a problem.

driis
+1  A: 

If possible it would probably be better if you threw wherever the failed result is set, otherwise you end up with having to do both result checking and throwing. But might not be possible of course.

Also, I'd make your error strings into resources or constants with placeholders so that you don't have to change multiple places if you want to change the wording.

ho1
+4  A: 

No problem... why would this be bad design?

Alternatively, since the exception type is the same in all cases, you could construct a lookup table for the error messages. That would save you some code duplication. For example:

static private Dictionary<int, string> errorMessages;
static
{
    // create and fill the Dictionary
}

// meanwhile, elsewhere in the code...
if (result is not ok) {
    throw new ClientException(string.Format(errorMessages[result], cTbx.Text, dTbx.Text));
}

In the messages themselves, you can select the appropriate parameter with {0}, {1}, etc.

Thomas
A: 

There is nothing wrong with the approach you are taking. Switch statements are a lot easier to read than if/then statements (and potentially faster too). The other thing you could do is load the possible exceptions in a

Dictionary<Result_Type, Exception>

and pull the exception from there. If you have lots of switch statements this would make the code more compact (and you can add and remove them at runtime if you need to).

Kevin
+9  A: 

Why not?

From The C# Programming Language, Third Ed. by Anders Hejlsberg et al, page 362:

The statement list of a switch section typically ends in a break, goto case, or goto default statement, but any construct that renders the end point of the statement list unreachable is permitted. [...] Likewise, a throw or return statement always transfers control elsewhere and never reaches its end point. Thus the following example is valid:

switch(i) {
case 0:
    while(true) F();
case 1:
    throw new ArgumentException();
case 2:
    return;
}
Daniel Pryden
A: 

I don't see any issue with using a switch in your case.

The stronger consideration should go to whether exceptions themselves are appropriate. Generally, exceptions should be used only when a situation arises that is outside the bounds of expected behavior. Exceptions shouldn't be used as program flow logic. In your case you're probably okay to use them based on the code I see.

KP
A: 

Maybe I beg to differ from all the answers here..

Instead of having to switch in the code, I would rather pass in the result to the ClientException class & let it decide what string it needs to show rather then have an ugly switch all over the place to create various messages

My code would look like:

throw new ClientException(result, cTbx.Text);

So, even if you can throw errors in switch...case, you can avoid it all is my take

Sunny
At which point the ClientException constructor would have same switch logic, but it would have to have breaks.
Yishai
or an if...else :)
Sunny
I downvoted because this "solution" just shifts the problem. This does not make any real difference.
Seventh Element
Sunny
In addition to the point @Yishai makes, the result might not always be an error code.
Rowland Shaw
Sunny
@Sunny I mean cases when the result was success, and as you should only really throw exceptions in exceptional circumstances, throwing an exception along the lines of "It worked" would occur, without a prior `switch...case` or `if...else...`, which would kinda defeat the idea of pushing the actual string lookup elsewhere
Rowland Shaw
@Rowland - in success cases, I am sure exceptions will not be thrown, my examples are for exception. I hope I am not mis-communicating in anyway about this.
Sunny