API design hall of shame…


Here is a new one for the API design Hall of Shame…. It has been in the .NET Framework from V1.0 days and I can’t believe I didn’t know about it until today… Maybe I did know about it before but I blocked it out of my memory 😉 


 


This API takes “true” and “false” rather than a Boolean value true and false…


http://msdn2.microsoft.com/en-us/library/system.appdomainsetup.shadowcopyfiles.aspx


 


public string ShadowCopyFiles { get; set; }


A String containing the string value “true” to indicate that shadow copying is turned on; or “false” to indicate that shadow copying is turned off.


 


 


Obviously, I would have MUCH preferred


public bool ShadowCopyFiles { get; set; }


 


 


So I checked with the team that owns the API to get the “background” story… While no one that still works in the area would claim “credit” for this API, I did get the story.  Apparently the rationale for using string was that the underlying infrastructure uses strings rather than bools.  This is a classic argument and one you should be very wary of whenever you hear it.  For the purpose of great API design you should separate your self from the implementation details for a moment and just ask “what would the user like\expect”.. and only deviate from that where there is a super compelling reason.  There clearly is no in this case.


 


 


Other suggestions for the API design all of shame?  Hopefully no new ones in .NET Framework 2.0.


 


   

Comments (24)

  1. Steven says:

    Can’t you just track down the guilty by looking in your source control system? Surely you’re running an ssc with history…

  2. David Betz says:

    One thing that I find weird is how the Bulleted List works. It does NOt take the ID from the datasource as it should making the DataValueField absolutely useless. I posted it as a bug and they came back with the usually "that’s by design" reply. Uh huh… that sooo would not pass usabiltiy tests. I need to be able to click on somethign and do something with it via id not a numeric sequence which means nothing to me. I either end up writing some custom solution or making a lookup take to translate the sequntial numbers it gives me to the REAL numbers which DataValueField needs to bind to.

    Anyhow… that’s been driving me insane for a long time,

  3. jmstall says:

    We’ve got a few embarrassing ones over in Debugger land.

    Your example is a case of it being "ugly". But there’s also a whole level of stupidity where the "design" makes something impossible.

    For example, there’s a design flaw in the debugging API such that we can’t report managed Exceptions that occur at startup. (Eg, if a thread throws a TypeLoad exception before it actually enters managed code. Details here: http://blogs.msdn.com/jmstall/archive/2005/08/01/Exception_Design_flaw.aspx )

    But at least this isn’t new in V2 🙂

  4. Lucas C says:

    Sorry Brad, yes here is a new one in v 2.0, at least in my view.

    It is about XPathExpression.Compile(string,IXmlNamespaceResolver), and I already submitted that at least the documentation should be clarified (FDBK40597).

    Whenever someone wants to use custom functions or variables in an XPath (but not XSLT) based solution, one has to pass a ‘context’ that derives from XsltContext to the expressions to be evaluated (the fact it is named XsltContext and not ‘XPathContext’ is in itself already wrong IMHO, to me that indicates that whoever designed the XPath parts was ignorant about the fact that XPath with variables and custom functions has plenty of uses outside XSLT).

    The overload of XPathExpression.Compile mentioned above seems to be the perfect way to pass that context, after all XsltContext implements IXmlNamespaceResolver. An innocent programmer may think that the following code would do the trick:

    MyXsltContext ctx = …;

    string xpathtext = …;

    XPathExpression expr = XPathExpression.Compile(xpathtext, MyXsltContext);

    Sadly, a look with Reflector shows that XPathExpression.Compile creates a *new* context, copying all namespaces from the one that was passed, and setting that new context (which of course has no knowledge of variables and custom functions) as the expression’s context…

    The way to get it working as intended is

    MyXsltContext ctx = …;

    string xpathtext = …;

    XPathExpression expr = XPathExpression.Compile(xpathtext, null);

    expr.SetContext(MyXsltContext);

  5. Yeah, I thought that one was kinda strange. Speaking of that, the docs say that AppDomain.SetShadowCopyPath is being deprecated and recommend using AppDomainSetup.ShadowCopyDirectories instead. The problem is that the SetupInformation on AppDomain gets a *copy* of the SetupInformation, so there is no way to modify the shadow copy path of an existing app domain. I have a scenario where this is useful, so right now, I’m forced to use the deprecated member. Do you know what I’m missing, or is this an API flaw?

  6. Chris Conti says:

    Not sure it counts, but why doesn’t treenodes and its ilk use the CheckState enumeration instead of a boolean property? The obvious answer is that the Windows Forms version of the control doesn’t support the tri-state checkbox…which of course begs the follow on question of why not?

  7. Battaile Fauber says:

    My nomination would be list item attributes only being rendered within htmlxxx controls, and not for radiobuttonlist, dropdownlist, etc. This was very counter-intuitive for me the first time I ran into it.

  8. Bart Jacobs says:

    Generic List is not sealed! Can you believe that?

    I was very happy to discover that List<>’s methods are non-virtual (contrary to ArrayList’s). The topping on the cake would have been if the class itself had been sealed.

    The most important downside of this is that it’s still possible to create a subclass of List<> that re-implements the IList<> or IList methods, with arbitrary or even malicious behavior.

    Secondarily, even barring method re-implementations, having a List<> object be unique defined by its elements would have been a nice breath of simplicity in an object-oriented world where people often go crazy with inheritance. (I think the BCL is generally fairly well-restrained in this respect, contrary to e.g. the Java APIs, and, indeed, this particular class.) Could a subclassed List<> object impact the behavior of serialization? I hope the answer is no but I fear the answer is yes.

    I think the .NET Framework could have used a type of lists (as in functional programming by the way). Period. A type List<T> whose semantics can be trusted to be that of "a list of values of type T". Not every class should be an extension point. And if any class should _not_ be an extension point, it’s List<>.

    When someone gets a List<T>, they should be able to assume that what they have is a list of values of type T. Sorry, maybe next time.

    If List<> objects were supposed to be facade objects, then yes. But that’s what IList<> is for. List<> is supposed to be a value object!

    I’ll bet the designers didn’t want to rule out the encoding of

    typedef List<Foo> MyListOfFoo;

    as

    sealed class MyListOfFoo : List<Foo> { /* ctors… */ }

    Which is a rather poor encoding anyway.

    I’d like to learn your opinion on this…

  9. Mike says:

    ShadowCopyFiles is worse than even you describe here. On v1.x, shadow copying will be enabled if ShadowCopyFiles is set to any non-null string value; even the empty string.

    So doing something like this:

    AppDomainSetup adSetup = new AppDomainSetup();

    adSetup.ShadowCopyFiles = "false";

    adSetup.ShadowCopyDirectories = "c:\blah";

    actually enables shadow copying, which is (to be polite) counterintuitive.

    This has been fixed in v2.0. Only "true" will enable shadow copying.

  10. Bart Jacobs says:

    As a follow-up to my earlier comment, I’d like to point out that this is yet another potential security vulnerability caused by a class not being sealed.

    It’s easy to imagine that someone writing a trusted library would accept a List<> object as the return value from an untrusted method and then assume that the values returned from the IList<>.Item property behave correctly (e.g. always return the same value). Note that a stack walk wouldn’t protect against this.

    The other vulnerability I’m referring to is that if a bad guy can declare a subclass D of a trusted class C, then they can create MemberwiseClone()s of D objects. I can easily imagine that the author of C assumes that they control instance creation, and base their security analysis on that assumption. E.g. when the object holds a counter of a maximum number of times some action may be performed. Cloning the object creates a new counter, allowing the action to be performed more often than anticipated.

    So, there’s another Hall of Shame hopeful for you: MemberwiseClone().

    More generally, I believe classes should have been sealed by default in C#. Instead of a sealed keyword, there should have been an extensible keyword.

  11. PatriotB says:

    Larry Osterman had a post some time back about GetBadWritePtr (and friends), and how they’re essentially useless and potentially damaging. Those would be good candiates for the hall of shame.

  12. Douglas McClean says:

    I vote for the entire System.Diagnostics.SymbolsStore namespace. I have tried five times and can’t figure out how to use it for anything (if it is even possible). What makes it worse is that it is so tempting, because it sounds like it would be very useful for reading .pdb information…

  13. I have a canditate for the hall of shame – DBNull class. Not serializable, pain-in-the-neck to deal with class that shouldn’t have existed in the first place (plain null reference would do).

  14. Kevin Dente says:

    Here’s one I just discovered:

    vsCommandStatusNinched

    Ninched?

  15. Here’s one I was just bitten by. This constructor for the Binding class

    Binding (String, Object, String, Boolean)

    The docs say that the last parameter "optionally enables formatting to be applied". What is not obvious is that the BindingComplete event does not fire either unless it is set to true!

  16. jmstall says:

    Douglas – see http://blogs.msdn.com/jmstall/archive/2005/08/25/pdb2xml.aspx for a sample that uses the symbol store interfaces to dump a PDB file into XML.

  17. sifbits says:

    How about Object.GetHashCode()? It seems to be there solely to make hashtables work, which is strange. The implications of overiding .Equals() is greatly complexified (is that a word) by the unnecessary coupling of that API with .GetHashCode(). In fact, there really is not a clear documented way to override .Equals in a mutable object that will cause the object to work correctly in a hashtable()!

    Why wasn’t .GetHashCode() left off of object and a different pattern chosen for hashtables, such as a required interface for objects added to hashtables or a way to refer to a hashcodebuilder? What percentage of object types in a typical application are actually used in a hashtable anyway? I would have preferred to make .Equals() much easier to override and just create an easy way to make objects hashable for a hashtable.

  18. Another one for the .NET 2.0 hall of shame:

    SqlDependecy.OnChange event

    OnChange event? Some one ignored the

    public event Event

    protected void OnEvent

    pattern.

    When it was discovered, it was too late to correct. Yeh!

  19. Andre de Cavaignac says:

    I got to say, speaking of the "that’s the way it was" argument, GDI+ must be the ugliest thing I’ve had to deal with in .NET. The "Rectangle" vs. "RectangleF" everywhere and the lack of structs being immutable is a real killer.

  20. Stefan Rusek says:

    Image.Palette is a horrible API. It does not work the way anyone I’ve ever talked to or worked with would have expected. As a result, almost everyone ends up giving up on trying to do indexed bitmaps. I’ve written more about it here.

    http://spaces.msn.com/members/stefanrusek/Blog/cns!1pmxsvG-1DbBBuajMN2jpMqw!192.entry

  21. kfarmer says:

    Actually, Bart, I’d say that the List interfaces are too monolithic, and need to be split apart.

    With the current set of interfaces, there’s no way to express whether or not an enumerable is finite in length or infinite, and if it’s finite, whether or not you can state the length ahead of time.

    Instead, you’ve got a bunch of APIs whose default behavior is to throw a not-implemented exception. Instead, I think, they should have been left off, and moved to interfaces that could optionally be implemented only on those classes for which they made sense. (For example, not on a serial port’s stream.)

    While on the subject of streams (and message queues.. let’s not forget those), couldn’t we have added an IEnumerable interface? I’m learning to love the usefulness of IEnumerable<T>, and the following would be very nice:

    SerialPort port = …;

    foreach (byte? b in port.Stream) // Stream -> IEnumerable<byte>

    {

    if (!port.IsOpen) break;

    Console.WriteLine("{0:x2} ", b.Value);

    }

    .. Though perhaps a stream would be better expressed as a Queue.

  22. PatriotB says:

    Here’s a good one: the last two values of this enumeration:

    http://msdn.microsoft.com/library/default.asp?url=/workshop/networking/pluggable/reference/enums/bindstring.asp

    (Nevermind the fact that most of the values are documented as "not currently supported".)

    Developers writing a pluggable protocol handler can call IInternetBindInfo::GetBindString to get various string values relating to the URL binding. This was introduced with IE4. Well, in IE5 there are some other types of data (non-string) you might want to get back. Rather than create a new interface with a new method, they overloaded this existing method to "string-ize" the values you might want.

    The documentation is so sparse it doesn’t even say what format these last two strings will be in. I happened to run across them in WinDbg and found out what they were. BINDSTRING_FLAG_BIND_TO_OBJECT returns a string "TRUE" or "FALSE". And BINDSTRING_PTR_BIND_CONTEXT returns a string of the address of the IBindCtx *in decimal form*, e.g. "1101284".

    Definitely API hall of shame material.