views:

709

answers:

11

I'm currently writing some code for UnconstrainedMelody which has generic methods to do with enums.

Now, I have a static class with a bunch of methods which are only meant to be used with "flags" enums. I can't add this as a constraint... so it's possible that they'll be called with other enum types too. In that case I'd like to throw an exception, but I'm not sure which one to throw.

Just to make this concrete, if I have something like this:

// Returns a value with all bits set by any values
public static T GetBitMask<T>() where T : struct, IEnumConstraint
{
    if (!IsFlags<T>()) // This method doesn't throw
    {
        throw new ???
    }
    // Normal work here
}

What's the best exception to throw? ArgumentException sounds logical, but it's a type argument rather than a normal argument, which could easily confuse things. Should I introduce my own TypeArgumentException class? Use InvalidOperationException? NotSupportedException? Anything else?

I'd rather not create my own exception for this unless it's clearly the right thing to do.

A: 

I'm always wary of writing custom exceptions, purely on the grounds that they aren't always documented clearly and cause confusion if not named correctly.

In this case I would throw an ArgumentException for the flags check failure. It's all down to preference really. Some coding standards I've seen go as far as to define which types of exceptions should be thrown in scenarios like this.

If the user was trying to pass in something which wasn't an enum then I would throw an InvalidOperationException.

Edit:

The others raise an interesting point that this is not supported. My only concern with a NotSupportedException is that generally those are the exceptions that get thrown when "dark matter" has been introduced to the system, or to put it another way, "This method must go into the system on this interface, but we won't turn it on until version 2.4"

I've also seen NotSupportedExceptions be thrown as a licensing exception "you're running the free version of this software, this function is not supported".

Edit 2:

Another possible one:

System.ComponentModel.InvalidEnumArgumentException

The exception thrown when using invalid arguments that are enumerators.

Peter
I'll have it constrained to be an enum (after some jiggery pokery) - it's only the flags I'm worried about.
Jon Skeet
I think those licensing guys should throw an instance of a `LicensingException` class inheriting from `InvalidOperationException`.
Mehrdad Afshari
I agree Mehrdad, Exceptions are unfortunately one of those areas where there is a lot of grey in the framework. But I'm sure this is the same for lots of languages. (not saying i'd go back to vb6's runtime error 13 hehe)
Peter
+7  A: 

I would use NotSupportedException as that is what you are saying. Other enums than the specific ones are not supported. This would of course be stated more clearly in the exception message.

Robban
NotSupportedException is used for a very different purpose in the BCL. It doesn't fit here. http://blogs.msdn.com/jaredpar/archive/2008/12/12/notimplementedexception-vs-notsupportedexception.aspx
JaredPar
+1  A: 

Id go with NotSupportedExpcetion.

Carl Bergquist
+7  A: 

I'd go with NotSupportedException. While ArgumentException looks fine, it's really expected when an argument passed to a method is unacceptable. A type argument is a defining characteristic for the actual method you want to call, not a real "argument." InvalidOperationException should be thrown when the operation you're performing can be valid in some cases but for the particular situation, it's unacceptable.

NotSupportedException is thrown when an operation is inherently unsupported. For instance, when implementing an interface where a particular member doesn't make sense for a class. This looks like a similar situation.

Mehrdad Afshari
Mmm. It still doesn't *quite* feel right, but I think it's going to be the closest thing to it.
Jon Skeet
Jon: it doesn't feel right because we naturally expect it to be catched by the compiler.
Mehrdad Afshari
Yup. This is an odd kind of constraint that I'd like to be applied but can't :)
Jon Skeet
+1  A: 

How about inheriting from NotSupportedException. While I agree with @Mehrdad that it makes the most sense, I hear your point that it doesn't seem to fit perfectly. So inherit from NotSupportedException, and that way people coding against your API can still catch a NotSupportedException.

BFree
+11  A: 

I would avoid NotSupportedException. This exception is used in the framework where a method is not implemented and there is a property indicating that this type of operation is not supported. It doesn't fit here

I think InvalidOperationException is the most appropriate exception you could throw here.

JaredPar
Thanks for the heads-up about NSE. Would welcome input from your colleagues too, btw...
Jon Skeet
The point is, the functionality Jon needs has nothing similar in the BCL. The compiler is supposed to catch it. If you remove the "property" requirement from NotSupportedException, things you mentioned (like ReadOnly collection) are the closest thing to Jon's issue.
Mehrdad Afshari
One point - I do have an IsFlags method (it has to be a method to be generic) which is *sort* of indicating that this type of operation is not supported... so in that sense NSE would be appropriate. i.e. the caller *can* check first.
Jon Skeet
@Jon: I think even if you don't have such a property **but** all members of your type inherently rely on the fact that `T` is an `enum` decorated with `Flags`, it would be valid to throw NSE.
Mehrdad Afshari
In fact I *could* just go ahead and give an answer anyway, in all cases - but I don't like giving meaningless results when the developer should be made aware that they're trying to do bitwise operations on a non-bitwise enum...
Jon Skeet
@Jon, I can see that argument. It still feels a bit wrong to use it because I worry that a static methods constrained to a type don't have the ... cohesive ... feel that a single interface does. It's the cohesive nature of the interface that allows for the relationship between the property indicating support and the method.
JaredPar
Yup. InvalidOperationException still doesn't feel right either though. Trouble is, I really don't like creating my own exceptions if I can help it. Hmm. Might just do it anyway - at least for the moment.
Jon Skeet
@Jon: `StupidClrException` makes a fun name ;)
Mehrdad Afshari
@Jon: Here's Brad Abrams's take on `NotImplementedException` and `NotSupportedException`: http://blogs.msdn.com/brada/archive/2004/07/29/201354.aspx
LukeH
My interpretation of the semantics of `NIE`/`NSE` is that throwing them shouldn't be contingent on the arguments used (whether standard or type arguments). A method is either implemented/supported or it isn't: If it doesn't throw `NIE`/`NSE` in *all* circumstances then it should *never* throw them.
LukeH
+8  A: 

Generic programming should not throw at runtime for invalid type parameters. It should not compile, you should have a compile time enforcement. I don't know what IsFlag<T>() contains, but perhaps you can turn this into a compile time enforcement, like trying to create a type that is only possible to create with 'flags'. Perhaps a traits class can help.

Update

If you must throw, I'd vote for InvalidOperationException. The reasoning is that generic types have parameters and errors related to (method) parameters are centered around the ArgumentException hierarchy. However, the recomendation on ArgumentException states that

if the failure does not involve the arguments themselves, then InvalidOperationException should be used.

There is at least one leap of faith in there, that method parameters recommendations are also to be applied to generic parameters, but there isn't anything better in the SystemException hierachy imho.

Remus Rusanu
No, there's no way that this can be constrained at compile time. `IsFlag<T>` determines whether the enum has `[FlagsAttribute]` applied to it, and the CLR doesn't have constraints based on attributes. It would in a perfect world - or there'd be some other way to constrain it - but in this case it just doesn't work :(
Jon Skeet
(+1 for the general principle though - I'd love to be *able* to constrain it.)
Jon Skeet
A: 

I would throw New ClassCastException("Custom message") but I'm weird that way.

Joshua
+9  A: 

NotSupportedException sounds like it plainly fits, but the documentation clearly states that it should be used for a different purpose. From the MSDN class remarks:

There are methods that are not supported in the base class, with the expectation that these methods will be implemented in the derived classes instead. The derived class might implement only a subset of the methods from the base class, and throw NotSupportedException for the unsupported methods.

Of course, there's a way in which NotSupportedException is obviously good enough, especially given its common-sense meaning. Having said that, I'm not sure if it's just right.

Given the purpose of Unconstrained Melody ...

There are various useful things that can be done with generic methods/classes where there's a type constraint of "T : enum" or "T : delegate" - but unfortunately, those are prohibited in C#.

This utility library works around the prohibitions using ildasm/ilasm ...

... it seems like a new Exception might be in order despite the high burden of proof we justly have to meet before creating custom Exceptions. Something like InvalidTypeParameterException might be useful throughout the library (or maybe not - this is surely an edge case, right?).

Will clients need to be able to distinguish this from BCL Exceptions? When might a client accidentally call this using a vanilla enum? How would you answer the questions posed by the accepted answer to What factors should be taken into consideration when writing a custom exception class?

Jeff Sternal
In fact it's almost tempting to throw an internal-only exception in the first place, in the same way that Code Contracts does... I don't believe anyone *should* be catching it.
Jon Skeet
Too bad it can't just return null!
Jeff Sternal
I'm going with TypeArgumentException.
Jon Skeet
+1  A: 

Throwing a custom made exception should always be done in any case where it is questionable. A custom exception will always work, regardless of the API users needs. The developer could catch either exception type if he does not care, but if the developer needs special handling he will be SOL.

eschneider
Also the developer should document all the exceptions thrown in the XML comments.
eschneider
A: 

I'd also vote for InvalidOperationException. I did an (incomplete) flowchart on .NET exception throwing guidelines based on Framework Design Guidelines 2nd Ed. awhile back if anyone's interested.

TrueWill