When is a good time to throw InvalidOperationException?

Question

I think I know what I mean about this but I am not quite sure...

The Framework documentation summarizes the type as follows:

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

There are clear-cut cases, and the one that comes to mind is when an operation requires an open database but the object hasn't been initialized with required information to connect to a database.

(Tangent: ADO.NETs behaivor of also requiring you to explicitly open the connection on the other hand is not as clear-cut; the DataAdapter deviates from this by simply opening the connection instad, closing it again if and only if it was closed at entry, and I find this convenient and have made myself an ADO.NET wrapper that uses this pattern for everything. Of course this means I risk doing 2 ExecuteNonQuery and needlessly returning the connection to the pool in between, but I can still open and close the connection when I want and this performance penalty is nothing compared to getting the exception.)

I guess the answer to my question is that it is ONLY in such clear-cut situations we should throw the exception. But what exception type would be most appropriate in the following scenario:

public class FormatterMapping
{
  Dictionary formattersByName = new ...();

  public IFormatter GetFormatter(string key) 
  {
     IFormatter f;
     if (formattersByName.TryGetValue(key, out f)) 
       return f;
     else
       throw new ??Exception("explanation of why this failed.");
  }
}

My first instinct was to throw ArgumentException. Then I started thinking that it might just as well be that the mapping is missing a key as the argument being "wrong". Basically the "Get formatter X" operation is invalid because X isn't in the mapping, but I don't really have a clue whether X "should have been there" or it's not sensible to ask for X here.

I could of course circumvent the whole issue by returning null, but that opens a bigger, deeper can of worms. There's no way to know when the return value will be used, so the code blowing up later with a NullReferenceException may not have an obvious relation to the place things went wrong. Either the mapping was badly set up, or the code using it asked for something it should not.

Another way to dodge the issue would be to head for the TryGetFormatter option, but the way I intend to use this the caller is in fact supposed to know what is and isn't in the mapping, so forcing this pattern on user code isn't good either.

Please don't answer I should just throw ApplicationException! And whatever you think the code should do, provide the reasons why. It is after all the reasoning that is really at issue here.

Until anyone convinces me otherwise, I'm leaning towards ArgumentException. From the point of view of the mapping the argument is wrong, so at least there's one clear line of reasoning supporting this. :)

Answer 1

Neither are perfect and both would be fine. Or perhaps you'd like something truly unambiguous:

KeyNotFoundException.

public class FormatterMapping
{
    Dictionary<string, IFormatter> formattersByName = new ...();

    public IFormatter GetFormatter(string key) 
    {
        // validate the argument
        if (!formattersByName.ContainsKey(key))
            throw new KeyNotFoundException("No formatter exists for given key");

        return formattersByName[key];
    }
}

Alternatively you could just let Dictionary<> throw it.

I suggest you pick one, document it and move on. It's not usually worth wasting a lot of time chosing specific exceptions to throw. Documenting the behavior is more important.

Answer 2

I would consider using ArgumentException for something like this instead:

if (string.IsNullOrEmpty(key))
{
    throw new ArgumentException("Expected a key");
}

For your example I think either InvalidOperationException or KeyNotFoundException would be suitable, or just write your own if you think it fits.

My opinion may not be liked by purists but my exceptions get emailed to me automatically in the system I work in, so ultimately in most cases I don't really care what type of exception I catch as long as I can get enough useful information out of it to see what happened. This includes:

1. An easy to understand error message
2. A stack trace
3. An inner exception if it is necessary
4. Any extra context information would be an optional bonus. By that I mean if you can capture any values around the time the exception was thrown it can make it much easier to debug.