4

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?

amon
  • 134,135
CHendrix
  • 514
  • 7
    "The requested page was not found. In addition, an error occurred while trying to find the 404 page to display." It happens. Do try to avoid it. – Lightness Races in Orbit Mar 07 '16 at 20:30
  • Note: You are using String.Concat. That, too, can throw an exception. – David Hammen Mar 07 '16 at 20:58
  • @DavidHammen I am not sure it will. The MSDN docs say "String.Empty is used in place of any null object in the array." and "An Empty string is used in place of any null argument." for various overloads. It seems like only if the parameter array itself is null . . . – Mike Mar 07 '16 at 21:29
  • @Mike -- Sorry. I used the wrong term. It can throw a Throwable, specifically, an OutOfMemoryError. – David Hammen Mar 07 '16 at 21:51
  • I'm so used to using a StringBuilder to build strings that I almost thought to suggest using that instead of String.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. – Berin Loritsch Nov 13 '17 at 16:26

1 Answers1

4

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.

Martin Ba
  • 7,608
  • 1
    In 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 Nov 13 '17 at 16:24