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. :)

Lazuli

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.

Barchan

I actually realized after posting that my example code would invite the answer "just leave it to the dictionary". In fact, the mapping supports a default formatter, but does not require one, and the throw situation comes about when the key is missing AND there is no default. – Lazuli

That said, KeyNotFoundException is perfect here. I'd forgotten it existed, and wanted to throw a framework exception (this is library code, and I'd like it to be as familiar as possible to people who've programmed .net for a while). Thanks! – Lazuli

My only gripe with the code is I really don't like having my code put the machine to do the same work twice, so I never use the Contains method unless ALL I want to know is if a key exists and I am not going to retrieve it anyway. The reason the TryGetValue method exists at all is precisely to provide a way to avoid this. I know the pattern looks a little unfamiliar, and I used to dislike having to use it (but not as much as the thought my code did 200% of the necessary work) but I've warmed to it since. – Lazuli

Fair enough. I'm not even sure why I rewrote it in the less efficient style... I just like the way it reads, I guess. Nothing wrong with your way at all. – Barchan

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.

Rogatory

Yes, of course - that is one of the clear-cut cases where the argument is invalid regardless of the state of the object. I should have included code for that, to avoid this kind of red herring. :) – Lazuli

Regarding "purist" or not: I think this really depends on what the code is meant to be. Library code needs to be stricter and more consistent than code in some app I've put together in half a day to make myself a basic but productive tool. When I make a tool for myself I can live with it only behaving correctly when I use it correctly. – Lazuli

If you're writing code for a library then I agree that exception types matter more. Finding a suitable existing exception type would be preferable to redesigning the wheel. But in the case of a library then what @igby said about documentation still matters more I think - a suitable exception name is good, but explaining exactly why it happens is better. – Rogatory