Throwing an exception of the proper type

Question

In my code I have often situations like this:

public void MyMethod(string data)
{
    AnotherClass objectOfAnotherClass = GetObject(data);
    if (objectOfAnotherClass == null)
        throw new WhatExceptionType1("objectOfAnotherClass is null.");

    if (objectOfAnotherClass.SomeProperty < 0)
        throw new WhatExceptionType2("SomeProperty must not be negative.");
}

Imagine that GetObject makes use of some external libraries which are not under my control and that this library returns null if no object for data exists and considers a negative SomeProperty as a valid state and therefore doesn't throw an exception. Further imagine that MyMethod cannot work without objectOfAnotherClass and does not make sense with a negative SomeProperty.

What are the proper exceptions for WhatExceptionType1/2 to throw in this situation?

Basically I had four options in mind:

• 1) InvalidOperationException, because MyMethod doesn't make sense under the conditions described above. On the other hand the guidelines (and Intellisense in VS too) say that an InvalidOperationException should be thrown if the object the method belongs to is in an invalid state. Now the object itself isn't in an invalid state. Instead the input parameter data and some other operations based on this parameter lead to a situation where MyMethod cannot operate anymore.

• 2) ArgumentException, because there are values for data the method can work with and other values the method can't. But I cannot check this by inspecting data alone, I have to call other operations before I decide.

• 3) Exception, because I don't know which other exception type to use and because all other predefined exceptions feel too specialized and not fitting to my situation.

• 4) MyCustomException (my own exception type derived from Exception). This seems always an option but I am worried that I have to define lots of special exception classes for many different error conditions when I start to follow this pattern.

Are there other and better options? What are the arguments in favor or against those options?

Thank you for feedback in advance!

Answer 1

I know this answer is far, far too late to be of use to the OP, but I'm inclined to think the difficulty here is more of a code smell. If we consider the Single Responsibility Principle at the level of a method instead of just a class, then it appears this method violates it. It does 2 things: it parses data and then does further processing. So the right thing to do is eliminate one responsibility, namely the parsing. You can do this by changing your argument to an instance of AnotherClass. Then it becomes clear what you should do:

public void MyMethod(AnotherClass data)
{
    if (data == null)
        throw new ArgumentNullException("data is null.");

    if (data.SomeProperty < 0)
        throw new ArgumentException("data.SomeProperty must not be negative.");

    ...
}

It then becomes the caller's responsibility to call the parsing method and deal with this situation. They may choose to catch the exception or pre-check it before calling the method. The caller has to do a little more work, but with the benefit of increased flexibility. For example, this also gives the benefit of allowing alternate constructions of AnotherClass; callers may build it up in code instead of parsing it or call some alternate parsing method.

Answer 2

I agree with the post of Stephen Cleary that you should try to categorize your exception and then find the Exception-type to throw accordingly. I find the categorization of Eric Lippert quite interesting and in many ways he is right, but, I've been thinking about another type of categorization that looks somewhat like the categorization of Lippert except that I focus more on code contracts. What I propose to do is to categorize Exceptions in failed preconditions, postconditions and plain errors.

The thing is: every method has a formal contract in the sense that if you satisfy all preconditions, it promises to satisfy some postconditions for you. Generally, all preconditions can be subdivided into three types:

• Security-related (is the caller authorized to make the call?)
• Context-related (is the object in the right state to make the call?)
• Input-related (has the caller specified valid arguments?)

In principle, every method should check these conditions in order and throw one of the following exception types (the exact type or derivative):

• SecurityException
• InvalidOperationException
• ArgumentException

These exceptions communicate to the caller that it was 'their fault' something went wrong and the call could not complete correctly. However, if all preconditions are satisfied according to the formal specification of the method, and the method detects it cannot meet the specified postconditions, it should throw an Exception of a type that clearly communicates what went wrong. You would typically want to define your own Exception type for these situations or re-use an existing Exception type, as long as it's not one of the types that are reserved for precondition-failures.

And finally, errors are Exceptions that are thrown by the CLR that can occur virtually anyplace, anytime, and are virtually impossible to predict. They will never be explicitly part of your methods and as such should never be thrown (neither specifically handled) by user code. In this sense, they map almost one-to-one to the fatal exceptions of Lippert.

So what should you do in my view in the case presented by Slauma?

Well, as you can imagine, it entirely depends on the contracts of MyMethod(data), GetObject(data) and objectOfAnotherClass.SomeProperty. What does the API of GetObject(data) say about in which situations it will return null (or throw it's own exceptions)? What is the exact meaning and specification of SomeProperty?

Suppose that GetObject(data) is a method that returns an object retrieved from a database and that data is the object's identifier. If the contract of GetObject(data) specifies that it will return null if no object with identifier data exists, then your design should take that into account in it's own contract. You might want to force the caller, if that's reasonable, to always specify a value for data such that GetObject(data) does not return null and if it does, you may throw an ArgumentException (or derivative) to indicate the fault on caller's side.

On the other hand, if GetObject(data) specified that it only returns null when the caller has insufficient rights to retrieve the object, you may throw a SecurityException (or derivative) to indicate the problem to the caller of MyMethod(data).

Finally, if GetObject(data) promises it will 'never' return null, and it still does, you may allow your code to crash with a NullReferenceException (because righteously assuming it will never be null) or handle the situation specifically when you are dealing with sensitive code, throwing your own Exception-type (since a postcondition, from the caller of MyMethod's perspective, has failed).

The second case is slightly more complex but can be approached much the same way. Suppose that objectOfAnotherClass presented a row or entity that was fetched from a database by identifier data, and MyMethod(data) clearly specified that data should specify an identifier of an object where objectOfAnotherClass.SomeProperty >= 0, then in this case throwing an ArgumentException (or derivative) would be part of your design.

Then again, when MyMethod(data) operates in a context where the caller may assume that a valid identifier will never return an object such that objectOfAnotherClass.SomeProperty < 0, (or better: is not even aware that such object exists) then such occurrence is truly unexpected on the caller's site and MyMethod(data) should either not even check the case explicitly (again, because assuming it will not occur) or, if going for more robust code, throw a specific, custom Exception indicating the problem.

To conclude: I think determining the type of Exception to throw solely depends on the formal contracts every method has with it's callers and callees. If the caller fails to satisfy a precondition, a method should always respond to the caller by throwing a SecurityException, InvalidOperationException or ArgumentException. If the method itself fails to meet it's own postconditions, or may either crash on exceptions thrown by the CLR or other components, or throw it's own more specific Exception indicating the problem.

Answer 3

Be careful when using built-in exception types... they have very specific meanings to the .NET framework, and unless you are using it for exactly the same meaning, it's better to roll your own (+1 to John Saunders for Serializeable).

InvalidOperationException has the meaning:

The exception that is thrown when a method call is invalid for the object's current state.

For example, if you call SqlConnection.Open(), you get an InvalidOperationException if you haven't specified a data source. InvalidOperationExceptionisn't appropriate for your scenario.

ArgumentException isn't appropriate, either. The failure to create objectOfAnotherClass may have nothing to do with bad data being passed in. Suppose it's a filename, but GetObject() doesn't have permissions to read the file. As the method is written, there's no way to know why the call to GetObject() failed, and the best you can say is the object returned was null or invalid.

Exception is just a bad idea, in general... it gives the caller absolutely no idea why the method failed to create the object. (For that matter, having only a catch (Exception ex) {..} is a bad idea, too)

You want exceptions that clearly identify what went wrong, and that means creating your own. Try to keep them generic to avoid 1,000 custom exceptions. I suggest:

ObjectCreateException:   // The call to GetObject() returned null<br />
InvalidObjectException:  // The object returned by GetObject() is invalid 
                         // (because the property < 0)

Thanks for the vote ~ thought I would add an example of some custom exceptions we've written.

Note that you don't really need to add any code to the methods, because the custom exceptions don't really do anything differently than their base classes; they just represent something different. The second example does add a property to the exception, so there's code to support it (including constructors).

The first is a generic base for all of our exceptions; the rule "Don't catch general Exceptions" applies (though we do it anyway... it allows us to differentiate between exceptions we generated and exceptions thrown by the framework). The second is a more specific exception that derives from Gs3Exception and serializes a custom property.

The .NET development team decided ApplicationException had no real-world value and deprecated it, but the purist in me always liked it, so it persists in my code. Here, though, it really does add no value and only increases the depth of the inheritance heirarchy. So feel free to inherit directly from Exception instead.

/// <summary>
/// A general, base error for GS3 applications </summary>
[Serializable]
public class Gs3Exception : ApplicationException {


    /// <summary>
    /// Initializes a new instance of the <see cref="Gs3Exception"/> class </summary>
    public Gs3Exception() {}


    /// <summary>
    /// Initializes a new instance of the <see cref="Gs3Exception"/> class </summary>
    /// <param name="message">A brief, descriptive message about the error </param>
    public Gs3Exception(string message) : base(message) {}


    /// <summary>
    /// Initializes a new instance of the <see cref="Gs3Exception"/> class 
    /// when deserializing </summary>
    /// <param name="info">The object that holds the serialized object data </param>
    /// <param name="context">The contextual information about the source or
    ///  destination.</param>
    public Gs3Exception(SerializationInfo info, StreamingContext context) : base(info, context) { }


    /// <summary>
    /// Initializes a new instance of the <see cref="Gs3Exception"/> class
    /// with a message and inner exception </summary>
    /// <param name="Message">A brief, descriptive message about the error </param>
    /// <param name="exc">The exception that triggered the failure </param>
    public Gs3Exception(string Message, Exception exc) : base(Message, exc) { }


}


/// <summary>
/// An object queried in an request was not found </summary>
[Serializable]
public class ObjectNotFoundException : Gs3Application {

    private string objectName = string.Empty;

    /// <summary>
    /// Initializes a new instance of the <see cref="ObjectNotFoundException"/> class </summary>
    public ObjectNotFoundException() {}


    /// <summary>
    /// Initializes a new instance of the <see cref="ObjectNotFoundException"/> class </summary>
    /// <param name="message">A brief, descriptive message about the error</param>
    public ObjectNotFoundException(string message) : base(message) {}


    /// <summary>
    /// Initializes a new instance of the <see cref="ObjectNotFoundException"/> class </summary>
    /// <param name="ObjectName">Name of the object not found </param>
    /// <param name="message">A brief, descriptive message about the error </param>
    public ObjectNotFoundException(string ObjectName, string message) : this(message) {
        this.ObjectName = ObjectName;
    }


    /// <summary>
    /// Initializes a new instance of the <see cref="ObjectNotFoundException"/> class.
    /// This method is used during deserialization to retrieve properties from 
    /// the serialized data. </summary>
    /// <param name="info">The object that holds the serialized object data.</param>
    /// <param name="context">The contextual information about the source or
    /// destination.</param>
    public ObjectNotFoundException(SerializationInfo info, StreamingContext context) : base(info, context) {
        if (null != info) {
            this.objectName = info.GetString("objectName");
        }
    }


    /// <summary>
    /// When serializing, sets the <see cref="T:System.Runtime.Serialization.SerializationInfo"/> 
    /// with information about the exception. </summary>
    /// <param name="info">The <see cref="T:System.Runtime.Serialization.SerializationInfo"/> that holds 
    /// the serialized object data about the exception being thrown.</param>
    /// <param name="context">The <see cref="T:System.Runtime.Serialization.StreamingContext"/> that contains contextual information about the source or destination.</param>
    /// <exception cref="T:System.ArgumentNullException">
    /// The <paramref name="info"/> parameter is a null reference (Nothing in Visual Basic) </exception>
    /// <PermissionSet>
    ///     <IPermission class="System.Security.Permissions.FileIOPermission, mscorlib, Version=2.0.3600.0, Culture=neutral, PublicKeyToken=b77a5c561934e089" version="1" Read="*AllFiles*" PathDiscovery="*AllFiles*"/>
    ///     <IPermission class="System.Security.Permissions.SecurityPermission, mscorlib, Version=2.0.3600.0, Culture=neutral, PublicKeyToken=b77a5c561934e089" version="1" Flags="SerializationFormatter"/>
    /// </PermissionSet>
    [SecurityPermissionAttribute(SecurityAction.LinkDemand, Flags=SecurityPermissionFlag.SerializationFormatter)]
    public override void GetObjectData(SerializationInfo info, StreamingContext context) {

        base.GetObjectData(info, context);

        //  'info' guaranteed to be non-null (base.GetObjectData() will throw an ArugmentNullException if it is)
        info.AddValue("objectName", this.objectName);

    }


    /// <summary>
    /// Gets or sets the name of the object not found </summary>
    /// <value>The name of the object </value>
    public string ObjectName {
        get { return objectName; }
        set { objectName = value; }
    }

}

PS: Before anyone calls me on it, the reason for a base Gs3Exception that adds no more value than the ApplicationException is the Enterprise Library Exception Handling Application Block... by having an application-level base exception, we can create general logging policies for exceptions thrown directly by our code.

Answer 4

The appropriate thing to do here is to not throw an exception if you can help it. There are two basic pieces of information that are missing to do the Right Thing. First there's the string argument, apparently the caller of this method has some secret knowledge of how GetObject() works to know what the appropriate string should be. You thus need to treat the caller as an authority, he knows a lot more about GetObject() than you do.

Second is the GetObject() behavior. Apparently the author designed it so that it is non-exceptional and expected for it to return a null. A much stronger contract would be two methods, TryGetObject() and GetObject(), the latter one throwing. Or more commonly an extra argument for GetObject() named throwOnFailure. But that didn't happen, null is returned and normal.

You are breaking that contract here. You are trying to turn it from non-exceptional to exceptional, but without having any clue what is wrong. The very best thing to do is to not change that contract and leave it up to the caller of your method deal with this. After all, that's the one that knows what GetObject() does. Change the name of the method, use the word "Try", and return a bool.

This is all assuming that the author of GetObject() knew what he was doing. If he didn't, there's little you can do to improve the situation. Throw ArgumentException if you have reason to think that the caller might have screwed up, NullReferenceException if you think that the GetObject() author might have a bug in his code.

Answer 5

First, categorize the error. Eric Lippert on his blog has the best categorization I've seen: fatal, boneheaded, vexing, and exogenous. Your exception would not be fatal; it would be one of: boneheaded, vexing, or exogenous.

The error is boneheaded if you can say that for a correct input name, you know that GetObject will return an object that makes sense for your method. In other words, the only cause for those exceptions is bugs in the code calling MyMethod. In this case, it doesn't really matter what exception type you use because you should never see it in production anyway - ArgumentException (if the problem was with name) or InvalidOperationException (if the problem was with the state of the object defining MyMethod) would be fine choices in that situation, but the specific exception type shouldn't be documented (or else it becomes part of the API).

The error is vexing if GetObject is predictable with respect to name (i.e., for a given name you will either always get a valid object or never get a valid object), but name is based on user input (or configuration). In this case, you don't want to throw an exception at all; you want to write a TryMyMethod method so that the calling code doesn't have to catch an exception to deal with a non-exceptional situation.

The error is exogenous if GetObject's behavior is unpredictable with respect to name: some external influence may cause name to become valid or invalid. In this case, you do have to choose a specific exception type and document it as part of the API. ArgumentException would not be a good choice for an exogenous exception; neither would InvalidOperationException. You should choose something more like FileNotFoundException, more descriptive of the underlying problem (whatever GetObject does).

Answer 6

It really depends on how you plan to handle exceptions in your appliation. Custom exceptions are nice in a try/catch situations but try/catches are also expensive. If you don't plan on catching and handling your custom exception, then: throw new Exception("Index out of range: SomeProperty must not be negative."); is just as useful as a custom exception.

public class InvalidStateException : ApplicationException
{
   ...
}

In your code:

// test for null
if(objectOfAnotherClass == null) throw new NullReferenceException("Object cannot be null");

// test for valid state
if(objectOfAnotherClass.SomeProperty < 0) throw new InvalidStateException("Object is in an invalid state");

Answer 7

If built-in exceptions exist that make sense, I would use those. If not, it makes sense to roll your own exception -- even if it's an empty class that extends Exception -- because this allows you to detect specific exception types. If you just threw Exception, for example, how do you know the exception was because objectOfAnotherClass was null, and that it wasn't some exception raised in GetObject?

So to summarize: specific exceptions are better, because you can (potentially) diagnose and recover from specific cases. Hence, use the built-in .NET exceptions (if they're sufficient), or else roll your own exceptions.

Edit: I should clarify that I rarely use existing exceptions and put a message in them. It makes your code more readable if the exception type tells you the error, rather than having to debug, generate the exception, and then inspect the message to see what the problem is.

Answer 8

My vote would be ArgumentException in at least the first case if not both; ArgumentExceptions should be thrown, quote, "when one of the arguments provided to a method is not valid". If MyMethod cannot use the argument data to create a valid instance of AnotherClass, as expected by MyMethod, the argument is invalid for use in MyMethod.

Understand that unless you plan on catching exceptions of different types and handling them differently, exactly which exception is thrown really doesn't matter. Some exceptions (like ArgumentNullException) create a custom message based on very little information and so are easy to set up and localize, others (like SqlExceptions) have more specific data about what went wrong, but all of that is superfluous if all you want is to throw out an exception saying "Oops!".

Answer 9

How about using the ObjectNotFoundException. That would describe the situation correctly.