9

With our public SDK, we tend to want to give very informative messages about why an exception occurs. For example:

if (interfaceInstance == null)
{
     string errMsg = string.Format(
          "Construction of Action Argument: {0}, via the empty constructor worked, but type: {1} could not be cast to type {2}.",
          ParameterInfo.Name,
          ParameterInfo.ParameterType,
          typeof(IParameter)
    );

    throw new InvalidOperationException(errMsg);
}

However, this tends to clutter up the flow of the code, as it tends to put a lot of focus on error messages rather than what the code is doing.

A colleague started refactoring some of the exception throwing to something like this:

if (interfaceInstance == null)
    throw EmptyConstructor();

...

private Exception EmptyConstructor()
{
    string errMsg = string.Format(
          "Construction of Action Argument: {0}, via the empty constructor worked, but type: {1} could not be cast to type {2}.",
          ParameterInfo.Name,
          ParameterInfo.ParameterType,
          typeof(IParameter)
    );

    return new InvalidOperationException(errMsg);
}

Which makes the code logic easier to understand, but adds a lot of extra methods to do error handling.

What are other ways to avoid the "long-exception-messages clutter logic" problem? I'm primarily asking about idiomatic C#/.NET, but how other languages manage it are helpful as well.

[Edit]

It would be nice to have the pros and cons of each approach as well.

  • 2
    IMHO your colleague's solution is a very good one, and I guess if you get really a lot of extra methods of that kind, you can reuse at least some of them. Having lots of small methods is fine when your methods are well named, as long as they create easy-to-understand building blocks of your program - this seems to be the case here. – Doc Brown Dec 12 '13 at 17:42
  • @DocBrown - Yeah, I like the idea (added as an answer below, along with pros/cons), but for both intellisense and for the potential number of methods, it begins to look like clutter as well. – FriendlyGuy Dec 12 '13 at 18:06
  • 1
    Thought: Don't make the message the carrier of all the exception detail. A combination of using the Exception.Data property, "picky" exception catching, calling code catching & adding its own context, along with the captured call stack all contribute information that should allow far less verbose messages. Finally System.Reflection.MethodBase looks promising for providing details to pass to your "exception construction" method. – radarbob Dec 16 '13 at 4:11
  • @radarbob are you suggesting that perhaps the exceptions are too verbose? Perhaps we're making it too much like logging. – FriendlyGuy Dec 16 '13 at 15:22
  • 1
    @MackleChan, I read that the paradigm here is to put info into a message that attempts to tell 'zactly what happened, is necessarily grammatically correct, and a pretense of AI: ".. via the empty constructor worked, but. .." Really? My inner forensic coder sees this as an evolutionary consequence of the common error of stack-trace-losing re-throws and unawareness of Exception.Data. The emphasis should be capturing telemetry. Refactoring here is fine, but it misses the problem. – radarbob Dec 18 '13 at 2:56
8

Why not have specialized exception classes?

if (interfaceInstance == null)
{
    throw new ThisParticularEmptyConstructorException(<maybe a couple parameters>);
}

That pushes the formatting and details to the exception itself, and leaves the main class uncluttered.

  • 1
    Pros: Very clean and organized, provides meaningful names for each exception. Cons: potentially a lot of extra exceptions classes. – FriendlyGuy Dec 12 '13 at 17:50
  • If it matters, you could have a limited number of exception classes, pass the class of where the exception originated as a parameter and have a giant switch inside more generic exception classes. But that has a faint smell to it - not sure I'd go there. – ptyx Dec 12 '13 at 17:57
  • that smells really bad (tightly coupled exceptions with classes). I guess it'd be hard to balance the customizable exception messages vs the number of exceptions. – FriendlyGuy Dec 13 '13 at 15:19
5

Microsoft seems to (via looking at the .NET source) sometimes use resource/Environment strings. For example, ParseDecimal:

throw new OverflowException(Environment.GetResourceString("Overflow_Decimal"));

Pros:

  • Centralizing exceptions messages, allowing for re-use
  • Keeping the exception message (arguably which don't matter to code) away from the logic of the methods
  • The type of exception being thrown is clear
  • Messages can be localized

Cons:

  • If one exception message is changed, they all change
  • The exception message is not as easily available to the code throwing the exception.
  • Message is static and contains no information about what values are wrong. If you want to format it, it's more clutter in the code.
  • 2
    You left out a huge benefit: Localization of exception text – 17 of 26 Dec 12 '13 at 17:02
  • @17of26 - Good point, added it. – FriendlyGuy Dec 12 '13 at 17:08
  • I upvoted your answer, but error messages made in this way are "static"; you can't add modifiers to them like the OP is doing in his code. So you've effectively left out some of his functionality. – Robert Harvey Dec 12 '13 at 17:18
  • @RobertHarvey - I added it as a con. That does explain why built-in exceptions never really give you local information. Also, FYI I am OP (I knew of this solution, but I wanted to know if others have better solutions). – FriendlyGuy Dec 12 '13 at 17:26
  • 4
    @17of26 as a developer, I hate localized exceptions with passion. I have to unlocalize them every time before I can eg. google for solutions. – Konrad Morawski Dec 12 '13 at 18:10
2

For the public SDK scenario, I would strongly consider using Microsoft Code Contracts as these provide informative errors, static checks and you can also generate documentation to add into XML docs and Sandcastle generated help files. It is supported in all paid for versions of Visual Studio.

An additional advantage is that if your customers are using C#, they can leverage your code contract reference assemblies to detect potential problems even before they run their code.

The full documentation for Code Contracts is here.

2

The technique I use is to combine, and outsource the validation and throwing altogether to a utility function.

The single most important benefit is that it is reduced down to a one-liner in the business logic.

I bet you can't do better unless you can reduce it further - to eliminate all argument validations and object-state guards from the business logic, keeping only the operational exceptional conditions.

There are, of course, ways to do that - Strongly typed language, "no invalid objects allowed anytime" design, Design by Contract, etc.

Example:

internal static class ValidationUtil
{
    internal static void ThrowIfRectNullOrInvalid(int imageWidth, int imageHeight, Rect rect)
    {
        if (rect == null)
        {
            throw new ArgumentNullException("rect");
        }
        if (rect.Right > imageWidth || rect.Bottom > imageHeight || MoonPhase.Now == MoonPhase.Invisible)
        {
            throw new ArgumentException(
                message: "This is uselessly informative",
                paramName: "rect");
        }
    }
}

public class Thing
{
    public void DoSomething(Rect rect)
    {
        ValidationUtil.ThrowIfRectNullOrInvalid(_imageWidth, _imageHeight, rect);
        // rest of your code
    }
}
0

[note] I copied this from the question into an answer in case there are comments about it.

Move each exception into a method of the class, taking in any arguments that need the formatting.

private Exception EmptyConstructor()
{
    string errMsg = string.Format(
          "Construction of Action Argument: {0}, via the empty constructor worked, but type: {1} could not be cast to type {2}.",
          ParameterInfo.Name,
          ParameterInfo.ParameterType,
          typeof(IParameter)
    );

    return new InvalidOperationException(errMsg);
}

Enclose all of the exception methods into region, and place them at the end of the class.

Pros:

  • Keeps the message out of the core logic of the method
  • Allows for adding logic information to each message (you can pass in arguments to the method)

Cons:

  • Method clutter. Potentially you could have a lot of methods that just return exceptions and aren't really related to the business logic.
  • Cannot reuse messages in other classes
  • I think the cons you cited far outweigh the advantages. – neontapir Dec 12 '13 at 18:34
  • @neontapir the cons are all easily addressed. Auxiliary methods should be given IntentionRevealingNames and grouped into some #region #endregion (which causes them to be hidden from the IDE by default), and if they are applicable across different classes, put them into a internal static ValidationUtility class. Btw, don't ever complain about long identifier names in front of a C# programmer. – rwong Feb 8 '15 at 0:19
  • I would complain about regions though. To my mind, if you find yourself wanting to resort to regions, the class probably had too many responsibilities. – neontapir Feb 8 '15 at 0:33
0

If you can get away with a little more general errors you could write a public static generic cast function for you, that infers the source type:

public static I CastOrThrow<I,T>(T t, string source)
{
    if (t is I)
        return (I)t;

    string errMsg = string.Format(
          "Failed to complete {0}, because type: {1} could not be cast to type {2}.",
          source,
          typeof(T),
          typeof(I)
        );

    throw new InvalidOperationException(errMsg);
}


/// and then:

var interfaceInstance = SdkHelper.CastTo<IParameter>(passedObject, "Action constructor");

There are variations possible (think SdkHelper.RequireNotNull()), that only check requirements on the inputs, and throw if they fail, but in this example combining the cast with producing the result is selfdocumenting and compact.

If you are on .net 4.5, there are ways to have the compiler insert the name of the current method/file as a method parameter (see CallerMemberAttibute). But for an SDK, you probably can't require your customers to switch to 4.5.

  • It's more about exceptions throwing in general (and managing the information in an exception vs how much it clutters the code), not about this specific example of casting. – FriendlyGuy Dec 12 '13 at 20:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.