Errata for the Framework Design Guidelines


We are finalizing plans to do a 2nd printing of the Framework Design Guidelines (thanks for those of you who bought a copy)…  As you may be aware, with additional printings they don’t generally like authors to add new content, but it does afford an opportunity to fix small errors.  Krys and I have found a few of on our own, but we’d love to hear your feedback as well.  What errors have you found?  While I can’t promise to send you $2.56 like Knuth does I will be grateful and will dually credit the first reports of such errors on my blog.


 


Thanks for your help!

Comments (29)

  1. Haacked says:

    Hi Brad. One thing I noticed is that in the basic dispose pattern, the guidelines make no mention of calling base.Dispose(bool); when overriding a class that uses this pattern. Wouldn’t that be necessary for the base class to clean up its resources?

    I wrote up the issue here: http://haacked.com/archive/2005/11/18/11222.aspx

    Also, can you call methods on your base class when within a finalizer?

  2. Haacked says:

    pg 265 first paragraph third sentence.

    Also, using optional features exposed <whoops!>though</whoops!> interfaces…

    I found another typo, but forgot to write it down and now can’t seem to find it.

  3. Joe Duffy says:

    Haacked, in response to the two issues you brought up on your first reply:

    (1) Failure to mention "chain-to-base" for Dispose;

    (2) Question about calling methods on base within Finalize.

    Re (1):

    If we neglect to say a derived class must chain to its base class Dispose method (if it has one), that’s a book-bug/omission. If you’re writing a Dispose(bool) your preference is to call base.Dispose(bool), flowing the bool value to the base method; but if one doesn’t exist, a base.Dispose() suffices; otherwise, you simply call base.Dispose().

    This is important because Dispose implies cleaning up resources; if you don’t chain to the base type, clearly you are going to leak those resources. And it might be worse than just nondeterministic cleanup (when the user expected deterministic). Presumably Dispose on the derived class does a GC.SuppressFinalize(this), meaning the base type will never get placed into the Finalize queue, and thus won’t release its resources (well, until process exit). The user of this class would notice this as unbounded resource consumption. I suspect the bug would be incredibly hard to find, too.

    Re (2):

    Generally, you should not make virtual method calls from your Finalize method. This is to avoid accidental reliance on a resource which has already been disposed during the destruction process. The original document from which the book text was derived acknowledged that Dispose’s use of this practice is risky and goes against the general advice. (Available here: http://www.bluebytesoftware.com/blog/PermaLink.aspx?guid=88e62cdf-5919-4ac7-bc33-20c06ae539ae)

    But we made an exception for Dispose because of the carefully controlled pattern. (This was in the original document.) And virtual calls during destruction aren’t nearly as dangerous as virtual calls during construction. You’re typically concerned that a resource will be used before it’s been initialized (i.e. in the construction case), but presumably code called from your Finalize will be resilient to uninitialized state and isn’t going to make further virtual methods (which might introduce problems). Clearly if this isn’t the case–and it could be difficult to verify that it is through test coverage–you will run into bugs, perhaps manifesting as crashing the Finalizer thread.

    joe

  4. Page 60 (comment in 2nd code block):

    // Image prefix is not necessary

    should be

    // ImageMode prefix is not necessary

  5. Nadim says:

    Not sure if this is an errata or just my lack of understanding, but I was surprised by the code example for 8.3, guideline 2 ("DO NOT use ArrayList or List<T> in public APIs").

    Showing the return of Collection<> vs List<> makes sense conceptually, but I would have expected to see ICollection<> instead, since List<> doesn’t derive from Collection<>, and you don’t want to imply snapshoting the internal list just to avoid exposing the richer type, right?

    -Nadim

  6. Shawn Cheng says:

    Second line on page 55, "… to read text from a file." This is not necessarily.

    I’m fine with the naming of Stream, StreamReader, TextReader, StringReader, and FileStream. Stream represents a sequence of bytes, and one uses StreamReader to create a Reader to read data from a stream. The name of StreamReader does capture the scenario of this usage. This does not have to have anything to do with inheritance hierarchy. Maybe the name just happens to imply that. Who knows what was in the mind of the .NET API designer at that time.

  7. shawn says:

    The comment from KRZYSZTOF CWALINA, "… or even the wrong type names…" It would be terrible to use wrong type names in public API.

    Shawn Cheng

  8. shawn says:

    On page 75, first sentence of third paragraph, "Finally, reference types are passed by reference, whereas value types are passed by by value."

    I would think that both reference and value types are passed by value (the value of the reference and the copy of the value type). One can pass both types by reference by using the ref keyword.

    Shawn Cheng

  9. shawn says:

    Sorry for not mentioning on which page KRZYSZTOF CWALINA gives the comment. It is page 54.

  10. shawn says:

    Section 4.4, "Abstract Class Design"~

    I would think that if the guideline treats "internal" as same as "public" as far as different users working on the same assembly is concerned, then the first "DO NOT" and "DO" should be:

    DO NOT define public or internal constructors in abstract classes.

    DO define a protected or a protected-internal constructor in abstract classes.

    If the guideline does not support "internal", then they should be:

    DO NOT define public constructors in abstract classes.

    DO define a protected constructor in abstract classes.

    Please do correct me if I’m wrong here.

    Shawn Cheng

  11. shawn says:

    It seems that the CONSIDER on page 55 is not in sync with the last paragraph on page 54. My opinion is that we should consider usage scenario first, then if it adds value, consider using the name of the base class as suffix.

    Shawn Cheng

  12. shawn says:

    Typos: page 77, two "get { … {" should be "get { … }

  13. shawn says:

    Page 99, "if ( … === … )".

    Shawn Cheng

  14. shawn says:

    I would think, to be more specific, the CONSIDER on page 87 should be:

    CONSIDER defining an interface if you need to support its functionality on classes that already inherit from some other class.

    Shawn Cheng

  15. shawn says:

    I guess the word "filed" from the last sentence on page 89 is a typo.

    Shawn Cheng

  16. shawn says:

    In my opinion, the first DO on page 89 is kind of strict to follow. Should it be a CONSIDER instead of DO?

    A framework should provide one or more implementations for the interfaces it defines, but it might not need to consume every its own interface in any way.

    Adding in extra APIs to the framework just to consume its interfaces is not a good solution to validate the interface design. The more we think about the purpose of those extra APIs, the more issues might be raised. For example, what access scope should we give to those APIs? If they are public, they can be invoked by the users of the framework. But do those APIs have solid purpose, so that the users want to call them? If the APIs are private, then the framework is forced to call them in some way, even if those extra APIs only serve as test cases. This will affect the design of the framework.

    In my opinion, the developers of the framework should provide test cases to consume the interfaces defined by the framework during development, and provide sample code (examples) to the users to show the scenarios to use the interfaces.

    Shawn Cheng

  17. shawn says:

    Page 171, the comment from JEFFREY RICHTER, "Close" (typo) should be "Clone".

    Shawn Cheng

  18. shawn says:

    On page 193, I would think there are typos in the comment from CHRISTOPHER BRUMME. The first sentence should read:

    "If you use [finally] clauses for cleanup, you should know that any code that comes after the end of the [finally] might not be executed."

    This happens when there is an exception. If you use catch clauses for cleanup instead, and if you can catch the exceptions concerned, your code after the catch will definitely run.

    Shawn Cheng

  19. shawn says:

    Not so sure whether this is a b-u-g, but could anyone please help point out the difference between "Do prefer" and "CONSIDER".

    Shawn Cheng

  20. shawn says:

    On page 197, I would agree with JEFFREY RICHTER at some degree, and would think that if the DO is changed to CONSIDER, many of us would feel better.

    Shawn Cheng

  21. shawn says:

    CONSIDER on page 186 refers to the wrong section.

    Shawn Cheng

  22. shawn says:

    Page 107, the comment from BRAD ABRAMS, I guess there is a typo in the last sentence, which should read:

    As you see in section 5.7.1, [an enum] argument would make it easier to understand code calling this API.

    Shawn Cheng

  23. shawn says:

    AVOID on page 156, the first sentence should read:

    "Using out or ref parameters requires experience with pointers, understand how pass-by-value and pass-by-reference differ, and handling methods with multiple return values.

    Shawn Cheng

  24. shawn says:

    I would suggest to give every single rule in the guideline a number for easy discussion.

    Shawn Cheng

  25. shawn says:

    Given section 5.7.5, "Pointer Parameters", I would think that the DO NOT on page 149 should be "AVOID" instead. Or it should be broken into two sub rules, one is DO NOT, another AVOID.

    Shawn Cheng

  26. shawn says:

    Typo: page 113, line 6, second word, "though", should be "through".

    Shawn Cheng

  27. shawn says:

    It seems that the CONSIDER (page 152) can be simplified to:

    CONSIDER using Booleans for constructor parameters that are simply used to initialize Boolean properties.

    Furthermore, it’s really hard to believe that some crazy guy wants to pass-in enum or integral values to initialize Boolean properties.

    Shawn Cheng

  28. shawn says:

    [page 152, ANTHONY MOORE]

    I can’t find CodeTypeReference.IsGlobal from MSDN help.

    Shawn Cheng

  29. shawn says:

    [page 153, first DO]

    As the example does not accept null as value for item, the method Add should check at the beginning whether item is null to throw ArgumentNullException.

    Shawn Cheng