rory.ap rory.ap - 1 month ago 9
C# Question

Should I throw an ArgumentException?

This question is about convention and interpreting MSDN, so I don't think it's primarily opinion based.

I'm wondering about ArgumentException: I have a builder class which is used to build a filter object that will be applied when retrieving a set of email messages from a mailbox. The builder has a number of methods to set the filter options. For example, I have two methods for setting the "sent date" range of the filter -- sent before XX and/or sent after XX. I want to add a guard clause to each which will throw an exception if, say, the provided "before" date is after the provided "after" date. I would do this with a common validation method:

/// <summary>
/// This class provides methods for validating dates in various ways.
/// </summary>
internal static class DateValidation
{
/// <summary>
/// Validate the provided "start" and "end" date/time values.
/// If they do not represent a valid range, throw an exception.
/// </summary>
/// <param name="start">The date/time that represents the start of the range.</param>
/// <param name="end">The date/time that represents the end of the range.</param>
internal static void ValidateDateTimeRange(DateTime? start, DateTime? end)
{
if (start.HasValue && end.HasValue)
{
if (start.Value > end.Value)
throw new Exception(
string.Format(@"The start date/time ""{0}"" " +
@"occurs after the end date/time ""{1}"".",
start.ToString(), end.ToString()));
}
}
}


Here are the two builder methods:

/// <summary>
/// Set a value which represents a date/time after which
/// messages must have been sent to be part of filtered output.
/// </summary>
/// <param name="afterDateTime">The date/time after which
/// messages must have been sent to be part of filtered output.</param>
public void SetSentAfterDateTime(DateTime afterDateTime)
{
DateValidation.ValidateDateTimeRange(afterDateTime, _sentBeforeDateTime);
_sentAfterDateTime = afterDateTime;
}

/// <summary>
/// Set a value which represents a date/time before which
/// messages must have been sent to be part of filtered output.
/// </summary>
/// <param name="beforeDateTime">The date/time before which
/// messages must have been sent to be part of filtered output.</param>
public void SetSentBeforeDateTime(DateTime beforeDateTime)
{
DateValidation.ValidateDateTimeRange(_sentAfterDateTime, beforeDateTime);
_sentBeforeDateTime = beforeDateTime;
}


According to MSDN:


Most commonly, an ArgumentException is thrown by the common language
runtime or another class library and indicates developer error.


I get that the phrase "most commonly" leaves the possibility open for other usages, like mine, but I like to stick to convention. I'm building a public API, so the exception will be documented and will bubble up beyond the public interface; furthermore, it doesn't "indicate developer error". It could conceivably indicate user error instead (it is a common convention to use exceptions to deal with user input validation issues). I get the feeling, based on MSDN description, though, that this is not what it is intended for.


...Derived classes [ArgumentNullException and
ArgumentOutOfRangeException] should be used instead of
ArgumentException, except in situations where neither of the derived
classes is acceptable. For example, exceptions should be thrown by:

...

ArgumentOutOfRangeException when the value of an argument is outside
the range of acceptable values; for example, when the value "46" is
passed as the month argument during the creation of a DateTime.


My argument might be outside the range of acceptable values, but that condition is determined dynamically based on the other date/time value. There is no static range of values that are "out of range".

So is
ArgumentException
typically used for cases like this?

Answer

From How to Design Exception Hierarchies by Krzysztof Cwalina:

A usage error is something that can be avoided by changing the code that calls your routine.

Let's agree that this is a usage error.

Eric Lippert calls them Boneheaded exceptions:

Boneheaded exceptions are your own darn fault, you could have prevented them and therefore they are bugs in your code. You should not catch them; doing so is hiding a bug in your code.

A developer should not catch these types of errors, but change the calling code in order to ensure the exception never happens in the first place. That is why the exact exception type does not matter much.

As Cwalina explains:

Usage errors need to be communicated to the human developer calling the routine. The exception type is not the best way to communicate errors to humans. The message string is a much better way. Therefore, for exceptions that are thrown as a result of usage errors, I would concentrate on designing a really good (that is explanatory) exception message and using one of the existing .NET Framework exception types: ArgumentNullException, ArgumentException, InvalidOperationException, NotSupportedException, etc. In other words, don’t create new exception types for usage errors because the type of the exception does not matter for usage errors. The .NET Framework already has enough types (actually too many, IMHO) to represent every usage error I can think of.

(Emphasis mine)

It doesn't really matter whether you're using an ArgumentException or an InvalidOperationException. Choose one, but make sure it has a clear Message.

Comments