Customized Exception

Question

I am building my "base exception to end all base exceptions" class in C# and was looking for some opinions on my implementations so far. Ideally my usage of integrated Rethrow, Throw, and ThrowFatal methods for some syntactic sugar.

using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Schloss
{
    [Serializable]
    public class SchlossException : Exception
    {
        // member variables
        private bool _fatal = false;
        private bool _rethrown = false;
        private ArrayList _additionalInfo = new ArrayList();

        public bool Fatal { get { return _fatal; } }
        public bool Rethrown { get { return _rethrown; } }
        public ArrayList AdditionalInfo { get { return _additionalInfo; } }

        public SchlossException() { }
        public SchlossException(string message) : base(message) { }
        public SchlossException(string message, Exception inner) : base(message, inner) { }
        protected SchlossException(
          System.Runtime.Serialization.SerializationInfo info,
          System.Runtime.Serialization.StreamingContext context)
            : base(info, context) { }

        // rethrow an exception to preserve the call stack across threads
        public void Rethrow()
        {
            _rethrown = true;

            this.Throw();
        }

        // throw this sexception
        public void Throw()
        {
            throw this;
        }

        // throw with the fatal flag, in case anyone is listening o_O
        public void ThrowFatal()
        {
            _fatal = true;

            this.Throw();
        }

        // poopy, messy code
        public SchlossException(string Msg, params object[] AddInfLst)
            : base(Msg, SchlossException.GetInnerException(AddInfLst))
        {
            this.AppendAdditionalInfo(AddInfLst);
        }

        private static Exception GetInnerException(params object[] AddInfLst)
        {
            Exception Exc = null;
            if (AddInfLst.Length > 1)
            {
                Exc = (AddInfLst[0] as Exception);
            }
            return Exc;
        }

        public void AppendAdditionalInfo(params object[] AddInfLst)
        {
            if (AddInfLst == null)
            {
                return;
            }
            for (int i = 0; i < AddInfLst.Length; i++)
            {
                object Obj = AddInfLst[i];
                if (Obj is object[])
                {
                    this.AppendAdditionalInfo((object[])Obj);
                }
                else
                {
                    this._additionalInfo.Add(Obj);
                }
            }
        }

        private string GetAdditionalInfo(ArrayList AddInfLst)
        {
            string AddInfStr = "";
            foreach (object Obj in AddInfLst)
            {
                string InfStr;
                if (Obj is Exception)
                {
                    InfStr = (Obj as Exception).ToString();
                }
                else
                {
                    if (Obj is ArrayList)
                    {
                        InfStr = this.GetAdditionalInfo(AddInfLst);
                    }
                    else
                    {
                        InfStr = Obj.ToString();
                    }
                }
                if (AddInfStr.Length > 0 && InfStr.Length > 0)
                {
                    AddInfStr += "\n";
                }
                AddInfStr += InfStr;
            }
            return AddInfStr;
        }

        public string GetMessageAndAdditionalInfo()
        {
            try
            {
                return this.GetBaseException().ToString() + " Additional Info: " + this.GetAdditionalInfo(this._additionalInfo);
            }
            catch
            {
                try
                {
                    return this.GetBaseException().ToString();
                }
                catch
                {
                    return "No Exception.";
                }
            }
        }
    }
}

Answer 1

Part One - Pink Glasses On

This as a qualifier

I like how you're using this in the Throw() method. However this is used as a qualifier in Rethrow() and ThrowFatal(), and the usage or non-usage of this as a qualifier seems inconsistent, especially with private fields being prefixed with an underscore. Member method calls don't need to be qualified like this, Throw(); is completely unambiguous all by itself.

Comments

I don't think any of the comments are useful. These ones deserves another look:

// throw this sexception

// poopy, messy code

Like I said I'd probably delete all comments anyway if I had to clean up that code, but these ones I'd delete twice. Call me a boring mug, I like professional code written professionally.

ArrayList

ArrayList is deprecated since .NET 2.0 blessed us with generics. Look into List<T> and IEnumerbale<T>!

Naming

Parameters should be camelCase;

public SchlossException(string Msg, params object[] AddInfLst)

Should be:

public SchlossException(string msg, params object[] addInfLst)

Actually, addInfLst means nothing to me, and looking at msg I realize it's just disemvoweled and it has nothing to do with an addIn. message and additionalInfo would have been so much better.

---

Part Two - Eyes Opened

I think the premise is wrong:

I am building my "base exception to end all base exceptions" class

A vast majority of all exceptions I throw are defined in the BCL. If you need to derive from your own exception classes all the time, and build a whole inheritance hierarchy of exception types... I'd say something's wrong: it's rather exceptional to throw an exception; throwing an exceptional one should be... exceptionally exceptional.

Throw & Rethrow

Rethrow should be ReThrow, but that's not the point: these two methods shouldn't exist at all. "Syntactic sugar", right. Every C# maintainer will expect an exception to be thrown/rethrown with the throw keyword. Calling an instance method on an exception type completely breaks the Principle Of Least Surprise, and makes me want to go to definition and see how it's implemented just to be sure it does what I'd expect it to be doing... and that's a sheer waste of time.

An exception doesn't throw or rethrow itself. With the throw keyword in the exception type, you're polluting the stack trace with useless information.

If I maintained code that uses this base class, I'd still be using the throw C# keyword, and wouldn't bother with these suspicious methods. Especially considering the actual syntax for correctly rethrowing an exception:

throw; // rethrows the same exception and keeps the same stack trace

Your Rethrow method actually sends the original stack trace to oblivion, and replaces it with one that's thrown.... inside the base exception class??

ThrowFatal

I'll be blunt, but any uncaught exception is fatal. You're twisting the semantics here, and I'm not sure I understand what this flag is supposed to be doing/meaning.

InnerException

From what I understand, you're using an object[] to store stuff, and in that stuff the index 0 might be an inner exception... but you're passing it as an actual inner exception through the base constructor.

You've just turned simple into woah-wait-a-minute-here. The inner exception is passed from your constructor, into the base constructor. That's it, that's all. It's not "additional info", it's an inner exception - it has nothing to do in that object[].

Additional Info

The idea of "additional info" might fly... except it's weakly typed, and will box up all value types - you should avoid boxing value types into reference types, even for an exception class. In fact, I think it's downright wrong; the "additional info" belongs in the derived types, as strongly-typed fields and exposed with well-named getters - this whole "additional info" array smells like your base class is trying to do the job of its derived types.. at least to me.

Answer 2

You're using ArrayList which is the weakly typed version of List<T>. Now, since apparently this is done because you're using object[], I would suggest to just strongly type this to List<object> instead.

This is assuming your additional information is in fact different unrelated types and not always a string, for example.

---

You're explicitly naming the System.Runtime.Serialization namespace instead of using a using statement. I don't immediately see any naming collisions so I would omit this verbosity.

---

this.Throw() is more verbose than it should be: you don't have to make any distinction between Throw() methods so you might as well call Throw().

---

Since Throw() only calls throw this;, I might be tempted to just omit the method altogether since you're adding another layer of complexity for really just two words.

---

Some comments should probably be rephrased in a professional environment as I'm sure you're aware.

---

Group constructors so they're all in once place.

---

Parameters are written in lowerCamelCase, so Method(string Msg) becomes Method(string message).

---

Don't abbreviate words. Msg becomes message, AddInfLst becomes additionalInfo.

---

There's no point in saying something is a list if you can look at the type and see it's.. an array? That illustrates the problem with this perfectly: your variable's name does not take changes to its type in account. It doesn't add any information that I didn't know from the type; in fact it even added confusion.

---

private static Exception GetInnerException(params object[] AddInfLst)
{
    Exception Exc = null;
    if (AddInfLst.Length > 1)
    {
        Exc = (AddInfLst[0] as Exception);
    }
    return Exc;
}

This is a curious piece of code. Let me sum up my frowns:

• Should this even be in this class? Why not a utility class? If I needed to get the innerexception from an object[], I'd look for something like ExceptionHelpers -- not SchlossException.GetInnerException().

• Is the first entry always the Exception? Apparently not because you use as which indicates that it may not be. What is the expected behaviour when index 1 has the exception?

• No documentation that explains how this method would work.

---

if (AddInfLst == null)
{
    return;
}

You don't need this statement since passing no arguments to a params method will result in an empty array. Therefore it will evaluate the loop but never enter it, nor throw a NullReferenceException on AddInfLst.Length.

---

Local variables use lowerCamelCase so Obj becomes obj.

---

It is better to follow this idiom:

var x = y as z;
if(x != null)
{
    // use x
} else {
    // use y
}

rather than this:

if(y is z)
{
    var x = y as z;
} else {
    // use y
}

It's a matter of 2 casts vs 1.

---

Create an empty string using string.Empty, it makes the intent more clear than "".

---

When you're looping, use a StringBuilder to concatenate strings instead of +=.

https://stackoverflow.com/questions/4191079/does-stringbuilder-use-more-memory-than-string-concatenation

---