views:

435

answers:

7

I have one method that looks like this:

void throwException(string msg)
{
    throw new MyException(msg);
}

Now if I write

int foo(int x, y)
{
    if (y == 0)
        throwException("Doh!");
    else
        return x/y;
}

the compiler will complain about foo that "not all paths return a value".

Is there an attribute I can add to throwException to avoid that ? Something like:

[NeverReturns]
void throwException(string msg)
{
    throw new MyException(msg);
}

I'm afraid custom attributes won't do, because for my purpose I'd need the cooperation of the compiler.

+9  A: 

No. I suggest you change the signature of your first function to return the exception rather than throw it, and leave the throw statement in your second function. That'll keep the compiler happy, and smells less bad as well.

David M
That's good practice? I've never seen anything done with an Exception object except throwing it.
Aviad P.
@Aviad P. No? Take a look here as to why that might be a good idea: http://stackoverflow.com/questions/1980044/when-should-i-use-a-throwhelper-method-instead-of-throwing-directly/1980078#1980078
Mark Seemann
@Aviad P. - I said smells less bad, not smells good. Check out the question Mark linked to.
David M
I'd say it's a good practice if you need to build the exception in a more complex way, like wrapping it and adding additional information.The method should be renamed. I can't see any point in having a method that just throws an exception.
PHeiberg
I would call this an exception factory, this doesn't smell bad at all.
Stefan Steinegger
Much thanks for this answer. It will be my accepted answer once I understand how to mark an answer as accepted ;-)
I think there's a tick next to it you can click on.
David M
I ended up flagging Christian Hayter's answer as accepted instead of yours, only because of his additional insight "functional code is better". Much thanks anyway.
Change my mind once more as you're the only one (of the two) to have explicitly answered my question about the attribute ;-) Now back to real work.
+26  A: 

Why not just change it to

int foo(int x, y)
{
    if (y == 0)
        throwException("Doh!");
    return x/y;
}

This gives the same runtime results, and the compiler won't complain.

Bernhof
Yes. This is the standard way of writing a code contract pattern pre CLR 4.0.
Christian Hayter
In this case I would probably do so, but `foo`was just an example. One of the call points looks like this: private int getVarID(string s_varID) { int varID; if (s_varID == "ILT") return 123; else if (s_varID == "TL") return 456; else if (s_varID == "FT") return 789; else if (int.TryParse(s_varID, out varID)) return varID; else throwParseError("varID must be an integer or 'ILT', 'TL' or 'FT'."); }
In that case I'd say you have other problems than "just" trying to get rid of a compile warning, if you say elsewhere that `throwParseError` does add contextual information to the exception thrown. (That must then be global context, right....?)
peSHIr
Both functions are methods of the same class and the added context is read from that class. What kind of "other problems" do you envision ?
This answer just bypasses my question, it does not answer it.
+3  A: 

Don't hand the exception creation off to another function (i.e. just throw it directly) and the compiler won't complain. Handing off to a "helper" type function for exception throwing is a waste of time unless the function is actually adding value to the exception process.

slugster
Correct. So in this case a simple `throw new MyException("Doh!");` would be in order. I usually only use such throwing methods when there is some logic involved; the (often validation) method could throw on some specific condition with a specific message or it could effectively do nothing.
peSHIr
Well, my example was just dumbed-down code. In the actual code, `throwException` adds context information to the exception.
+1  A: 

Bernhof already gave you a way to avoid the compiler complain. However, also be aware your stack trace will be off (and some logger libraries won't handle util-classed-i-throw-exceptions-for-your-app methods) which makes debugging your application harder.

Rick
Thanks for the insight. Here it will be less of a problem I guess because the 2 functions belong to the same class. Would David M's solution (exception created in separate function and thrown in `foo`) avoid the problem ?
@fred-hh (answering myself) yes (this is documented).
A: 

Remove the 'else' keyword, it's redundant anyway, and it will work ;)

Fedor Hajdu
+1  A: 

Your function

void throwException(string msg)
{
    throw new MyException(msg);
}

add zero value to the code, hence your question is moot. If, on the other hand you want to throw an error with the same message throughout the class and minimise code duplication this is what you should do.

The normal practice would be to extend MyException for this particular case and throw that:

public class HomerSimpsonException : MyException
{
   public HomerSimpsonException() : base ("DOH!!!"){
   }
}
int foo(int x, y)
{
    if (y == 0)
        throw new HomerSimpsonException();
    else
        return x/y;
}

Even then, that's not complete enough as per Microsoft rule for extending exceptions, there are minimum 4 constructors that you should implement - http://msdn.microsoft.com/en-us/library/ms182151%28VS.80%29.aspx, namely:

  public NewException(){}
  public NewException(string){}
  public NewException(string, Exception){}
  protected or private NewException(SerializationInfo, StreamingContext){}
Igor Zevaka
+3  A: 

Bernhof's answer is correct. However, if you are trying to encapsulate a large chunk of logic when instantiating your exception, then all you need to do is change your code from this:

void throwException(string msg) {
    throw new MyException(msg);
}

to this:

Exception makeException(string msg) {
    return new MyException(msg);
}

Then your calling code will look like this:

int foo(int x, y) {
    if (y == 0) {
        throw makeException("Doh!");
    }
    return x / y;
}

All other things being equal, prefer functional code to procedural code. It's easier to re-use and unit-test.

EDIT:

In light of Fred's sample code, this is what I would do. It's not a code contract, but it's still functional.

private int getVarID(string s_varID) {
    int varID;
    if(s_varID == "ILT") {
        return 123;
    } else if(s_varID == "TL") {
        return 456;
    } else if(s_varID == "FT") {
        return 789;
    } else if(int.TryParse(s_varID, out varID)) {
        return varID;
    } else {
        throw makeParseError("varID must be an integer or 'ILT', 'TL' or 'FT'.");
    }
}
Christian Hayter
Much thanks for your answer. Almost got to be accepted. But there can be at most one accepted answer... Thanks also to remind me that "prefer functional code to procedural code. It's easier to re-use and unit-test", though in this case it won't change much (the throw/makeException method will stay short.)