Skip to main content
Source Link
Jeroen Vannevel
  • 11.6k
  • 2
  • 38
  • 79

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 +=.

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