ArgumentNullException and ArgumentException


In my comments Jeff asks:


I have a question, how was the decision for these 2 classes ctor parameters made…


Constructor for ArgumentNullException


public ArgumentNullException( string paramName, string message)


 


Constructor for ArgumentException


public ArgumentException( string message, string paramName)


 


Well, in short Jeff, this was a design mistake.  In fact it is up on the internal “API Design Hall of Shame”.  Here is the text from that site:


 


ArgumentNullException does not follow the constructor pattern given in the Design Guidelines spec.
Result: Habit wins out and people commonly type:
throw new ArgumentNullException (“must pass an employee name”);
rather than:
throw new ArgumentNullException (“Name”, “must pass an employee name”);
so we end up with error message such as:
Unhandled Exception: System.ArgumentNullException: Value cannot be null. Parameter name: must pass employee name
Lesson: Just follow the pattern!


 


Got any other examples that should be in my API Design Hall of Shame?

Comments (35)

  1. David Bossert says:

    "Got any other examples that should be in my API Design Hall of Shame?"

    Just off the top of my head:

    AppDomain vs. ApplicationDomain

    MarshalByRefObject vs. MarshalByReferenceObject

    STAThreadAttribute vs. StaThreadAttribute

    ObjRef vs. ObjectReference

    …and so on. If I had my way, all of these annoying little naming mishaps would be fixed without notice and anyone who complained about compatibility would risk being beaten with a coat hanger… but that’s just me.

  2. My candidate to the Hall of Shame is IDbDataAdapter.Fill.

    There is no override that would take a DataSet and a table name making it difficult to use it with typed datasets. In order to fill typed datasets, my database provider agnostic data layer has to use reflection to invoke the provider specific Fill with the table name parameter.

  3. Mike says:

    It is declared as

    public static string SmtpServer {get; set;}

    The MSDN documentation states:

    Thread Safety

    Any public static (Shared in Visual Basic) members of this type are safe for multithreaded operations. Any instance members are not guaranteed to be thread safe.

    But, to send a message you need to set SmtpServer first (static property), and then send a message (static method). How should one do this "safe for multithreaded operations"?

    Of course, if I write standalone application, this is not a problem – I can lock something. But if there is other code running in same process which can wish to send e-mail too, I am out of luck.

  4. RichB says:

    Any method in System.Security.Cryptography. Having written .Net code since 2000, it was a shock recently to use the Crypto APIs and find how bad they are.

  5. RichB says:

    CLSCompliantAttribute

    – Doesn’t follow the naming conventions. Should be ClsCompliantAttribute

  6. Ben says:

    I was wondering if there was any way to see this list – I think it’d be a great read. Wouldn’t it also be neat to set a forum-like system up for the public to post their little oddities that they have found (like those posted here)?

  7. Kevin Westhead says:

    System.Windows.Forms.FileDialog has always annoyed me. This class is defined as:

    public abstract class FileDialog : CommonDialog

    However the documentation states:

    "FileDialog is an abstract class, and cannot be created directly. Additionally, you cannot inherit from this class."

    This seemingly contradictory statement is in fact true because FileDialog contains the following method:

    internal abstract bool RunFileDialog(OPENFILENAME_I ofn);

    The design here seems wrong and it is annoying when you have expectations (based on experience) about what you can do with a class defined as "public abstract", only to find out that the class behaves differently.

    It is documented at least, which I suppose is something since you end up looking through the docs eventually. <g>

  8. _ says:

    RichB,

    >> CLSCompliantAttribute

    >> – Doesn’t follow the naming conventions. Should be ClsCompliantAttribute

    It does follow the conventions, according to http://blogs.msdn.com/brada/archive/2004/02/07/69436.aspx

    Seeya

  9. Brad Abrams says:

    Nope, Kevin is right, it should be ClsCompliant… Three letters or more == Pascal cased.

  10. Atif Aziz says:

    It’s interesting to note that a lot of complaints about casing issues can be fixed with a small feature that exists in the C# compiler today: Aliasing. That is, you can rename a type explicitly as you import it:

    #region System aliases

    using ClsCompliantAttribute = System.CLSCompliantAttribute;

    using ApplicationDomain = System.AppDomain;

    using MarshalByReferenceObject = System.MarshalByRefObject;

    using StaThreadAttribute = System.STAThreadAttribute;

    using ObjectReference = System.Runtime.Remoting.ObjRef;

    #endregion

    This is also do-able in Visual Basic.NET, which goes even further than C# if you dig into the books. What I never understood, on the other hand, is why the VB team left out aliasing of interfaces! Anyhow, just wanted to let some VB guy reading this comment to know that this solution is applicable there too.

    Of course, it should not be our job to fix-up such mistakes and the improted list could look rather ugly at the beginning of each file (which is why it’s good to hide it into a collapsed region), but it’s nonethelss an option to consider if it really kills you to write AppDomain instead of ApplicationDomain.

    By the way, aliasing is nice even if you don’t want to rename a type. It is very useful for reducing the type noice in your scope. For example, why import the whole System.IO namespace if all you need is some method of the Path class? Here you would use aliasing as follows:

    using Path = System.IO.Path;

    Personally, I can live with such mistakes in the BCL with the hope that it will be fixed through some ingenious scheme of the CLR, compilers, the IDE or tools like FxCop. Hey, I’ve lived with some atrocities in Win32 for a big part of my career that the .NET Framework is pleasant experience any day. The quality bar though has to be raised for the next time as it was raised by the BCL with the first release. Looking at this blog tells me so far that someone is watching this space and a big effort is being made to pay attention to such details and fix them for the future. That’s a good thing and keep up the great work, Brad. What’s scary is the sheer size of the design guidelines, which seems to be growing by the day. This is a clear indication that we need to make things simpler, perhaps even question some of the basic notions as we understand them today, in order to make the next big leap.

  11. Dwayne says:

    Here’s one for the hall of shame.

    There is no base data access exception for the data access libraries. For example, the System.Data.SqlClient.SqlException derives from System.SystemException. So does the System.Data.OracleClient.OracleException class. This makes it more difficult to create a common data access interface that uses these classes internally but throws an implementation agnostic exception. It would have been nice if they had a System.Data.DataException that these exceptions derived from, so that my interfaces could be consistent.

  12. Dwayne says:

    The System.Collections API.

    I’m surprised that the System.Collections.ICollection does not even have a Contains(object) method.

  13. Dwayne says:

    Actually, the most annoying thing about the System.Collections API is that CollectionBase implements IList. Most of the methods/properties in IList would be better defined on ICollection IMHO. At very least the Add, Clear, Contains, Insert, and Remove methods, as well as the IsReadOnly property.

  14. Tom McKearney says:

    Well, it exists. THey’re just not using it the way you’d like.

    http://msdn.microsoft.com/library/en-us/cpref/html/frlrfsystemdatadataexceptionclasstopic.asp

  15. Dwayne says:

    Good point Tom, that’ll teach me to write my response without first looking at the APIs. The point I was making still stands though. There is no common base exception type for the database-implementation specific exceptions that can identify the exception as being one generated by the database.

  16. Atif Aziz says:

    Actually, talking about Exceptions, I think that the whole exception hierarchy is unreliable and broken in the .NET Framework. By unreliable, I mean that I cannot use it to make the right choices about the base exceptions and write code that can gracefully and predictably recover from a given situation while keeping forward and backward compatibility in mind. I think there is no real good story here. When is something just an Exception, when is it a SystemException and when is it an ApplicationException? I know that right thing for my own exception hierarchy is to base it off ApplicationException, but when I look at the .NET Framework, I get the impression that the various teams could not always agree on the best place to branch off their exception hierarchy. For example, some exceptions from the framework are based off ApplicationException. What makes things complicated are things like ArgumentException. This one inherits from SystemException, which means that if I inherit from it, then I am essentially making my exception a system one. The documentation states that, "[SystemException] is provided as a means to differentiate between exceptions defined by the system versus exceptions defined by applications." So the question is when are you a system and when are you an application? ArgumentException is probably not the best example because, frankly, no one would catch it at runtime since it probably means that you haven’t tested things correctly and are just being plain paranoid. So a better case in point might be System.IO.IOException. If I am writing an I/O library and I want to create an exception hierarchy, should I base it off System.IO.IOException (which by the way inherits from SystemException) or create my own IOException that inherits from ApplicationException? Who is the system here? Is the SystemException space reserved for Microsoft? If that’s the case, then the compilers should not allow anyone to inherit from SystemException or Exception.

    Perhaps a better way to solve this problem would have been through attributes or marker interfaces. I don’t believe that OO hierarchies are the best candidates for dealing with exceptions and the versioning issues that arise from it. The lack of consistency and a good story is what makes even some otherwise very clever piece of code to do something scary like this:

    try

    {



    }

    catch (Exception e)

    {



    }

    Frankly, if you’re not re-throwing the exception in the catch block then you’ve just broken everything because you’d end up even swalloing ridiculous exceptions like StackOverflowException, or God forbid, ExecutionEngineException. I can’t imagine what this do for Yukon where various pieces of managed code need to run in a server reliably and 24 by 7. If they’re swallowing system exceptions, then they’re essentially hiding pretty grave situations from the host that normally lead to, say, the termination of their AppDomain.

    Luckily, FXCop has a design rule that checks for the following violation: "You should not catch Exception or SystemException." However, I’ve found that in practice, you are at the pure mercy of how someone designed their exception hierarchy. If they based it off System.Exception, then you’re left with no real choices. Also, use of FxCop is best practice and not mandatory.

    Given this situation, I would be inclined to put the whole exception hierarchy into the API Design Hall of Shame.

  17. Diego Mijelshon says:

    Some of the biggest mistakes in API design are in the Collections Framework:

    Why in the world System.Array implements IList.Contains as private?

    Why do Arrays have a Length property instead of Count? (or vice-versa)

    I could go on and on…

  18. Ken Brubaker says:

    For my own reference, I thought I’d compile a quick list of design guidelines added by Brad Abrams, et al.

  19. Brian Grunkemeyer says:

    On these two questions:

    *) Why in the world System.Array implements IList.Contains as private?

    The thought was it was just syntactic sugar around IndexOf, or for sorted arrays, BinarySearch. Users figure this out themselves, and if they know their array is sorted, they can do this more efficiently than we could by simply calling the BinarySearch method. And if we didn’t need to expose it, why add the surface area?

    I was toying with suggesting that we make Contains public on Array, but I’m not sure that’s the best decision for the product. In our Whidbey release, to support generic programming in a more natural way than C++’s Standard Template Library, we made T[] implement IList<T>. Of course there’s a Contains method on IList<T>, which ends up calling Array’s new IndexOf<T>. This can theoretically give better performance (with the right IL code generators and the appropriate work in our current JIT and/or a future JIT – measuring on any publically released V2 build right now is probably not a useful exercise). However, since we only made some arrays, not all arrays, implement IList<T>, we didn’t define a Contains<T> on the Array class directly (there is one subtle internal hack where we violate the type system to make the type system useful for generic programming). So we haven’t accidentally solved your feature request yet.

    But there’s no technical reason we couldn’t add an Array.Contains<T> that is a simple wrapper around Array.IndexOf<T>. You would still be better off calling BinarySearch<T> if your array is sorted though. I’ll run this by a few people – maybe this would be interesting & useful.

    *) Why do Arrays have a Length property instead of Count? (or vice-versa)

    Length made more sense for arrays, where they are always a series of values in memory. You can pin them & do pointer arithmetic on arrays of value types, etc. They’re similar to C arrays, and we had that mentality firmly ingrained in us.

    Collections can include data structures like trees or an infinite series of prime numbers. Neither of these has a length, and only one has a finite count. The naming issue here was too much to swallow, so we left arrays with Length and collections with Count. Yes, it’s a bit confusing, but if this is the worst of the problems our users are running into, we’ve done a good job.

  20. Ken Brubaker says:

    Documentation bug for HttpServerUtility.Transfer

  21. Diego Mijelshon says:

    Brian,

    You’re being a little too defensive with your product. Believe me, I’m not trying to destroy it; I just want to help you make it even better 🙂

    I just think it wouldn’t hurt to have IList.Contains and IList.Count marked public instead of private.

    I don’t care if they actually call !(Array.IndexOf(this, value) < this.GetLowerBound(0)) and this.Length. That’s just an implementation detail that shouln’t change the API.

  22. Atif Aziz says:

    HttpException and all dervied exceptions are not marked as serializable! I can’t imagine any reason for them not to be serializable. It’s not like I want to persist the exceptions to disk, but lack of serializability in these classes means that they can’t get tossed across remoting boundaries such as AppDomains and contexts. What’s more, why on Earth do they inherit from System.Runtime.InteropServices.ExternalException, which is supposed to be, accoriding to the docs, the base exception type for all COM interop exceptions and structured exception handling (SEH) exceptions. Shame, shame, shame. 🙂 BTW, although Mono follows or has to follow the same exception hierarchy, it does at least mark HttpException as serializable.

  23. Brad Abrams says:

    Good point, Atif… I talked wth the dev team for HttpException. You will be happy to know they are making it serializable for Whidbey. The ExternalException base class is an oddness, but is harder to change.

  24. Brad Abrams says:

    Mike, on the issues with SmtpServer, you will be happy to know that in Whidbey we will be adding a much better mail support to the platform that does, I am told, fix this problem.

  25. Ilya Ryzhenkov says:

    My favorite is internal bool EndsWith(char value) in String class. Why it would be internal? Also, there is no bool StartsWith(char value).

  26. Mike Krings says:

    Publishing WMI Classes is nearly unrecoverable in case of a failure:

    If the WMI class is not yet registered and the user is not in the Admin group, a simple System.Exception is thrown. No real chance to gracefully handle this, as the only hint on the source of the error is the error message.

    If publishing fails because of an Security exception, the Exception raised is a COMException (shouldn’t it be a ComException ?) with a WMI specific (not System.Management published) HRESULT of 0x80041003.

    Looks like a real hasty transformation of the WMI API.

  27. RichB says:

    Random.Next(0,1);

    does not return numbers in the set {0..1}. It returns numbers in the set {0..0}. Arghh!

  28. Kit George says:

    With regards to Ilya’s question (why not add EndsWith/Startswith overloads that accept a char), there’s simply not much of a gain here. We actually have a feature request for this, but it’s lower down on our priority stack.

    On Rich’s Random.Next issue: yup, this is an unfortunate one. But basically, we can’t change this behavior, for breaking change reasons. However, I just entered a feature request for us to consider either a new method, or a new overload (consider obsoleting this one).

  29. Why this singleton System.Reflection.Missing has the constructor internal instead of private? Same question for the internal class System.Empty.

  30. Brian Grunkemeyer says:

    About the Missing & Empty constructors – there is no real difference. Whether the constructors are internal or private has no impact to the public API, especially since these classes are private. I suspect this was required in our original design, but never got fixed when we revamped these classes, about the same time we removed Variant from the public namespace about 2.5 years into writing V1. Either that, or it was an earlier compiler limitation. (We went through two separate compilers for V1 before we ported our source to C#.)

    But this is a little odd, and I’ll make these constructors are private.