5
\$\begingroup\$

Because I don't like writing the same useless exeption messages all the time ;-) I thought I create a better exception that would make this for me. This is what I came up with:

[Serializable]
public class SmartException : Exception
{
    private const string PropertyNamePrefix = "$";
    private const string InnerExceptionSeparator = " >>> ";

    protected SmartException() { }

    protected SmartException(Exception innerException) : base(null, innerException) { }

    public override string Message => FormatMessage();

    protected T GetValue<T>([CallerMemberName] string propertyName = "")
    {
        return (T)Data[PropertyNamePrefix + propertyName];
    }

    protected void SetValue<T>(T value, [CallerMemberName] string propertyName = "")
    {
        Data[PropertyNamePrefix + propertyName] = value;
    }

    private string FormatMessage()
    {
        var ex = (Exception)this;

        var message = new StringBuilder(1024);


        while (ex != null)
        {
            var exceptionSeparator = message.Length > 0 ? InnerExceptionSeparator : string.Empty;
            var exceptionName = Regex.Replace(ex.GetType().Name, "Exception$", string.Empty);
            message.Append($"{exceptionSeparator}{exceptionName}");

            var properties = FormatProperties(ex);
            message.Append(string.IsNullOrEmpty(properties) ? null : $": {properties}");

            ex = ex.InnerException;
        }

        return message.ToString();
    }

    private static string FormatProperties(Exception ex)
    {
        return
            ex is SmartException
            ? FormatPropertiesFromData(ex)
            : FormatPropertiesFromType(ex);
    }

    private static string FormatPropertiesFromData(Exception ex)
    {
        var entries = ex.Data
            .Cast<DictionaryEntry>()
            .Where(de =>
                de.Key is string &&
                ((string)de.Key).StartsWith("$") &&
                de.Value != null);

        var properties = new StringBuilder(1024);
        foreach (var entry in entries)
        {
            var separator = properties.Length > 0 ? " " : string.Empty;
            var propertyName = Regex.Replace(
                entry.Key.ToString(), 
                $"^[{PropertyNamePrefix}]", 
                string.Empty);

            properties.Append($"{separator}{propertyName} = \"{entry.Value}\"");
        }

        return properties.ToString();
    }

    private static string FormatPropertiesFromType(Exception ex)
    {
        var propertyInfos =
            ex.GetType()
            .GetProperties(BindingFlags.DeclaredOnly | BindingFlags.Instance | BindingFlags.Public)
            .Where(p => p.Name != "Message");

        var properties = new StringBuilder(1024);

        foreach (var propertyInfo in propertyInfos)
        {
            var value = propertyInfo.GetValue(ex);
            if (value == null)
            {
                continue;
            }
            var separator = properties.Length > 0 ? " " : string.Empty;
            properties.Append($"{separator}{propertyInfo.Name} = \"{value}\"");
        }

        return properties.ToString();
    }
}

Usage

public class UserNotFoundException : SmartException
{
    public UserNotFoundException(Exception innerException, [CallerMemberName] string callerName = "") : base(innerException)
    {
        CallerName = callerName;
    }

    public string CallerName
    {
        get { return GetValue<string>(); }
        private set { SetValue(value); }
    }

    public string UserName
    {
        get { return GetValue<string>(); }
        set { SetValue(value); }
    }
}

and for:

var innerEx = new FileNotFoundException(null, "helloworld.txt");
var ex = new UserNotFoundException(innerEx) { UserName = "TestUser" };

the message would be:

UserNotFound: CallerName = "Foo" UserName = "TestUser" >>> FileNotFound: FileName = "helloworld.txt"

What it does is cutting-off the Exception from the name of the exception type and appending the properties to the message together with all inner exceptions.

\$\endgroup\$
4
\$\begingroup\$

Whilst your code is a clever little trick to add useful details to an exception, there is one vital flaw as I see it, and that is you don't know where the exception was thrown. In the best code bases I've seen and worked with, there is always an unique exception message which you later on can use to locate where the specific exception was thrown.

In mostly stable production code, exceptions should not occur very often, but if they occur two things are important when looking into it:

  1. Details as to why the exception occured, and this your code does good. You make it easy to provide good details to the exception.
  2. Where the exception occured, and this you seem to bypass. You only have a generic reference to which exception was thrown and possibly a somewhat unique combination of parameters. This will make it hard for you to locate your exception when your code base is getting larger

Addendum: Unique exception messages

One way to generate unique message is to repeat something related to the method/function you are executing and the failing situation. Lets say that your missing file was within creation of a user account, then I would have preferred a message like:

UserNotFound: Failed creating user account - CallerName: "Foo", UserName: "TestUser" >>> FileNotFound: Some message - FileName: "helloworld.txt"

Now when you, as a developer or support, gets a hold of the exception you should be able to locate where this exception occured in your code base by a single search for "Failed creating user account". This message is not dynamic, but unique. The properties are dynamic, and should add details to the exception.

With such an exception you can locate the offending code even though the stack trace is not available, which it wouldn't be if the exception is found is shown in a dialog window or status bar, or in some cases in a log file, or if it being copy-pasted from somewhere into an email.

The unique message is also something you can train users to respond to. They can now search in a given list where they can follow some procedures, whilst for other (and unknown exceptions) they contact support.

One final note on unique messages, even a unique number, would be better than nothing at all. Just don't let it be autogenerated which renders it useless when you rebuild your code. Some code bases I've seen has numbering schemes like Exxyyzzz where xx can denote module, yy file in module, and zzz could be a running number within that file.

\$\endgroup\$
  • \$\begingroup\$ *2 - You mean the way how I create the inner FileNotFounException? This is only for testing. Later I will of course throw it correctly to keep the stack trace. \$\endgroup\$ – t3chb0t Nov 3 '15 at 19:31
  • \$\begingroup\$ @t3chb0t, I mean that the exception should in it's pure text form contain something uniquely identifying across your code base. Often exceptions would be output to a log file (without stacktrace), to an alert windows (also without stack trace), or maybe in an email based on something the user saw somewhere. With a unique message, you then still know where to look for the offending code \$\endgroup\$ – holroy Nov 3 '15 at 19:34
  • \$\begingroup\$ There are no useful messages ;-) The default .net messages are completely useless and if they aren't they just repeat the same things that the exeption type name already says... so why not make it autogenerated? Like the FileNotFoundException. It even doesn't print the file name in its message. \$\endgroup\$ – t3chb0t Nov 3 '15 at 19:37
  • 1
    \$\begingroup\$ @t3chb0t, And that is why you should add a useful unique message to your exception! :-) \$\endgroup\$ – holroy Nov 3 '15 at 19:39
  • \$\begingroup\$ But what else can I say that the properties and the exception type cannot? It is probably for a longer discussion but it think writing "An exeption occured while trying to read a file" says exacly the same as if I created an exception and called it FileReadException :-] \$\endgroup\$ – t3chb0t Nov 3 '15 at 19:42
4
\$\begingroup\$

Code Styling

Your code is very good at first look. Good structure, code-styling, new features used, clever trics.

Functionality and Usability

The message is for the user, the code is for you to debug it. Exceptions currently allow adding good message and naming properties won't help a bit (to the user, if it is designed to tell the user: "You did something wrong" like "Wrong Value Format"), but to diagnose problems, you'll need more than property/method name. And for that, you have the stack trace.

To state it simple: the problem is not in the exception and what additional info it can carry (it already has the stack-trace), but in the way you present it. This is my own code I use:

public static string GetLongMessage(this Exception ex)
{
    StringBuilder sb = new StringBuilder();
    while(ex != null)
    {
        sb.AppendLine(ex.Message).AppendLine();
        sb.AppendLine(ex.StackTrace).AppendLine().AppendLine();
        ex = ex.InnerException;
    }
    return sb.ToString();
}

Example (added)

I have purposedly created bad XML file and opened it in my app. The result is not so perfect (for a user, because they should never hand-edit the XML), but I can clearly see the problem:

enter image description here

You can see, what is wrong, in the title - File Open Error And can track the exceptions to see what elements and what attribute is wrong (page-state-badattr). It won't tell much to the user (except for the title - could not properly parse selected file), but I know, immediatelly. Actually, the stack trace is not that important here, but the exception chain is perfect!

But to achieve this, I had to create custom exceptions and add additional data (element/attribute name) to them. But that should be normal :)

\$\endgroup\$
  • 2
    \$\begingroup\$ While I was working on this exception utility I asked myself too, who is the one to read the message? So I asked it here on Programmers - as it turns out, messages are for developers ;-) \$\endgroup\$ – t3chb0t Nov 3 '15 at 19:39
  • \$\begingroup\$ I agree. I use the code to log something or show big message box with the content (if it bubbles far enough to be real problem). Users will printscreen it and send it to me to diagnose. I look and know, repair and release ;) \$\endgroup\$ – user52292 Nov 3 '15 at 19:45

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.