As I was TDD-ing some code, it occurred to me that one of my typed exceptions could potentially throw a NullReferenceException
while creating its error message.
Here's the constructor for the typed exception in question:
public class MyTypedException : Exception
{
public MyTypedException(String expectedValue, ICollection<String> possibleValues)
{
String message = String.Format("Failed to find {0}. Possible values:", expectedValue);
foreach(var value in possibleValues)
{
message = String.Concat(message, ", ", value);
}
/* Assigns message to property, etc... */
}
}
If possibleValues
is null, then the constructor of MyTypedException
will throw a NullReferenceException
. My first reaction was to think "Well, just check if possibleValues
is null, and if it is, throw an ArgumentNullException
. However, throwing a different exception from the constructor of an excpetion just seems...wrong.
The alternative would be to check whether possibleValues
is null or empty, and if it is, create a different message, like so:
public MyTypedException(String expectedValue, ICollection<String> possibleValues)
{
String message = String.Empty;
if (possibleValues == null || possibleValues.Count <= 0)
message = "Failed to find {0} because the collection of possibles values is null or empty";
else
{
message = String.Format("Failed to find {0}. Possible values:", expectedValue);
foreach (var value in possibleValues)
{
message = String.Concat(message, ", ", value);
}
}
/* Assigns message to property, etc... */
}
Is it ever okay to throw a different exception when attempting to create an exception?
1 Answer 1
IMHO, it is not OK.
When you throw a MyTypedException
, the most important aspect is that this error happened. Secondary errors or usage bugs might be of interest of course, but they are secondary and masking the original one is a really bad idea I think.
Of course, one cannot avoid all errors (esp. like the mentioned OutOfMemory
or similar), but barring fatal errors (like OOM, etc.) knowingly throwing something other than the intended Exception doesn't seem like a good idea to me.
Do note: In your example, if ArgumentNullException
indicates a bug in the calling code, then other means of catching the bug are of course a Good Thing:
- You can
Assert
and then - just continue in the production code with the above workaround
- additionally do some warning/debug logging
- and/or
FailFast
if you think this bug is critical. (I wouldn't.)
Also note that delaying any formatting of the message to later might also make sense (just keep a ref to the passed arguments, and format on request.)
Not (manually) formatting at all, hence avoiding the whole problem of the example might even be better.
-
1In this particular case, I would recognize that not providing any
possibleValues
is something that is possible. The whole "Possible Values: " part of the message would then need to only be added if there are actually possible values (i.e.possibleValues != null && !possibleValues.isEmpty()
.Berin Loritsch– Berin Loritsch11/13/2017 16:24:03Commented Nov 13, 2017 at 16:24
String.Concat
. That, too, can throw an exception.Throwable
, specifically, anOutOfMemoryError
.StringBuilder
to build strings that I almost thought to suggest using that instead ofString.Concat()
, but if you are throwing enough of these exceptions where that actually makes a difference then there are much bigger problems.... Your corrected version of the exception is what I would expect to see.