This is part three of the series.
Type Design (continued…)
p 89: Struct design: DO ensure that a state where all instance data is set to zero … is valid. This sounds reasonable, until they give an example. They consider a structure
struct PositiveInteger
{
int value;
}and suggest that the design that directly holds integer value in the value field is a Bad Thing, because value would be 0 by default, and it is not valid.
Suggested Good Thing is to have “logical value”, which is value plus one, i.e.
struct PositiveInteger
{
int value;
public PositiveInteger( int value )
{
this.value = value - 1;
}
public string ToString()
{
return (value+1).ToString();
}
}
I think in this particular case that kind of trickery will cause more harm than good. I’d rather accept invalid values.
p 92: As we’ve seen in other studies, if an API makes it look to them [less experienced developers] as though a specific scenario or requirement isn’t immediately possible, it’s likely that they will change the requirement and do what does appear to be possible, rather than being motivated to spend time investigating what they need to do to achieve the original goal. These are bad, lazy developers. 🙂 I would make sure they understand it is not OK to change requirements on a whim. And if that does not work, I would avoid working with such developers.
p 95: Did you know that the CLR supports enums with the underlying type of float or double? No, I did not. Wow… even though most languages choose not to expose it. A-a-ah…
p 100: It is generally acceptable to add values to enums unless you expose them through a web service.
Member Design
p 109: In fact, whenever you have to use literal null in your code when calling an API, it indicates an error in your code or in the framework not providing the appropriate overload. This seems a little harsh to me. Providing gazillion overloads is not always the answer.
p 115: You must code with the expectation that the subclass will be hostile. It seemed a little paranoid to me at first, but then I realized this is the sad truth. Subclasses have privileged access to their parents, so if your library needs to enforce security, subclasses, as any other caller, should be treated as suspect.
p 115: Choosing between properties and methods. They discuss
public class PersonFinder
{
public string FindPerson( int height, int weight, ..., Connection database);
}
versus
public class PersonFinder
{
public int Height { get; set; }
public int Weight { get; set; }
...
public string FindPersonName( Connection database );
}The guideline favors the second approach (lots of properties) with some descending opinions. In my view, both approaches are bad. If person lookup criteria becomes complicated, we need to separate building criteria from the finding operation itself. This, by the way, usually produces better code when you need to pass the criteria around.
public class PersonCriteria
{
public int Height { get; set; }
...
}
public class PersonFinder
{
public string FindPerson( PersonCriteria criteria, Connection database );
}
p 118: DO use a method, rather than a property if … the operation returns a different result each time it is called.
Jeffrey Richter: DateTime.Now should have been a method. Good call, Jeffrey!
p 118: Brian Pepin: Handle property on the Windows Forms control is an example where too many side effect might occur. Go Brian! I told you these guys know what they are talking about. 🙂
p 120: Use properties for simple access to simple data with a simple access. Don’t stray from that pattern. Too bad Microsoft developers don’t always follow their own advice. I know of at least one bug in .NET framework (related to Windows Forms scaling) that partially arises from use of properties where methods should have been used.
p 138: Event handlers must be of the form BlaEventHandler( object sender, BlaEventArgs e ); Why? People always ask this. In this end, this is just about a pattern. By having event arguments packaged in a class you get better versioning semantics. I don’t buy the “just about a pattern” argument. Why don’t we make all the methods look like void SomeMethod( int operation, ref object data )? Just for the hack of it. It would be a nice pattern. 🙂 And it would be much easier for inexperienced developers, since all methods look the same. The “better versioning semantics” argument carries more weight. However, it is still not clear why I must have a sender and especially why it must be object.
p 139: Do not provide instance fields that are public or protected. Well, it seems to be a general consensus that public fields are a bad idea. However, many people (but not myself) seem to think that protected fields are OK. The main argument against public fields in the book is that they if need be, they cannot be changed to properties. From this perspective public fields are not very different from protected fields – both may be used by code outside of class author’s control.
p 141: Do not assign instances of mutable types to readonly fields. They don’t really say why. I assume the idea is that people will confuse “readonly” and “immutable”. For reference types, it is the reference that is immutable, but not what it points to. This is all neat, but what if I do mean to say that the reference never changes? “Readonly” is the only way to achieve that. Besides, some tools (e.g. resharper) prompt you to mark a field readonly if it can be made readonly.
p. 143: Do not be cute when defining operator overloads… It is not appropriate… to use the shift operator to write to a stream. This very thinly veiled allusion to C++ is understandable, but it makes the whole guideline quite questionable. As broken as C++ streams may be, usage of the shift operator is by no means a serious problem.
p. 148: There is no point in allowing users to make calls that won’t work. I could not agree more. Of course, not all requirements may be expressed at compile-time, but those that can be expressed, should be expressed.
p. 148: Do not use reserved parameters. Hear, hear. I wish Win32 API people listened to this. Of course, Win32 API is C, and C does not have overloads, but reserved parameters are evil anyway.
p 153: Do validate arguments passed to public, protected, or explicitly implemented members. There is no point in allowing users to make calls that won’t work 🙂
p.153: Do not user Enum.IsDefined for enum range checks.. This is somewhat surprising. After all, if I want to know that an enum value is legal, what do I do? They say Enum.IsDefined has lousy performance. Well, that may be true. But they never say what should I use instead.
p. 156: Avoid using out or ref parameters. I totally agree for public methods. For private methods, however, they are sometimes useful and make the code simpler.
Extensibility
p. 164: Keep in mind that it’s usually possible to add more extensibility later, but you can never take it away without introducing breaking changes. While the message is right, this literal statement is debatable. If API was designed with total disregard of extensibility, it may be quite difficult to add.
p. 164: The main reasons framework users often want to inherit from unsealed classes is to add convenience members, such as custom cosntructors, new methods or method overloads. This is a questionable idea. If you want to add custom constructors, use factory methods. If you want convenience members, use utility classes or extension methods from .NET 3.5. If instances of a class are passed to and from the API, users start casting instances of base class to derived class, because “we know it is going to be our derived class”.
p. 167: The main disadvantage of callbacks is that they are quite heavyweight in comparison to virtual members. The main disadvantage of callbacks is that they break the linearity of code and make it difficult to follow. Performance considerations are rarely an issue.
p. 169: Do not make members virtual unless you have a good reason to do so. Here I agree. Java’s idea of making everything virtual by default is kinda problematic. From the other hand, sometimes it allows to gracefully extended classes in interesting ways.
p. 169: The peril: If you ship types with virtual members you are promising to forever abide by subtle and complex observable behaviors and subclass interactions. Very true. In other words virtual methods are dangerous. Virtual methods are a form of inversion of control. Derived class is not a master of its own fate. It should rely on the base class to call it when it sees fit. This is what makes the interactions subtle and not trivial.
I have seen all kinds of issues with deep hierarchies, especially deep hierarchies of GUI classes. I have seen situations when derived class wants to inherit the visual aspects of the base, but fights very hard to get rid of the implementation, by creative use of overrides (this is also known as the “refused bequest” smell). Replacing inheritance with composition usually makes all these problems go away. Former derived class is now in control. It calls (delegates to) the former base when needed, and things are straight again.
Exception handling
p. 181: It is better to terminate the application than to let it muddle on – at leas the error is eventually fixed.. Some people take this principle too far and declare that all exceptions must be fatal (and I do know such people). This is, of course, total nonsense. Let’s say I am writing something like a development environment, something similar to Visual Studio, or Eclipse. I send document to a printer, and it causes an exception. It is quite obvious that the menu handler for File->Print should handle most printing exceptions itself and let the program continue. Even if the printer learned to fly and relocated to Mars, this is not good enough reason to terminate my development work and lose all my data.
p. 185:For example, a method called ReadByte shoudl throw if there are no more bytes left… A method named ReadChar should not throw…, because EOF is a valid char. This would be very, very confusing. I’d say, throw in both cases.
p. 187:Do not have public members that can either throw or not based on some option. This is probably a good thing. Having methods like that make error handling confusing.
p. 187: Do not have public members that return exceptions as the return value or an out parameter. “Do not” is too strong a rule here. For some asynchronous operations it is important to keep exceptions at bay for a while, and throw them only on correct thread, etc. However, these cases are rare, and frequently we can work around this problem by using EndXXX kind of call, that re-throws stored exception. The exception, therefore, remains private and is not returned by public API.
p. 188: Avoid explicitly throwing exceptions from finally blocks. Implicitly thrown exceptions resulting from calling methods that throw are acceptable. I don’t see the point here. Probably, what they wanted to say is “don’t throw from finally blocks unconditionally”. This would be, indeed, a dubious idea.
p. 190: Consider localizing the exception messages. This is a bad idea. One day you, the framework developer, will receive an error report full of funny squiggly characters of some foreign language, and you will have to make sense of it. Jeffrey Richter backs me up on this.
p. 190: Exception messages are not relevant when an exception is handled; they only come into play when an exception is unhandled. This is not true. Even handled exceptions are often logged (see the printer example above).
p. 195: When in doubt, do not wrap an exception with another. An example is… TargetInvocationException. This is incredibly annoying becuase it hides the actual method … that had a problem. I have wasted much time trying to debug my code because of this exception wrapping. I could not agree more. Besides debugging wows, TargetInvocationException pointlessly changes the exception type, so catching a particular exception becomes too hard. Another example is the exception that CAB throws when an event topic handler causes an exception. As far as I remember, they don’t even bother to include the causing exception in the message.
p. 197: Do not throw or derive from ApplicationException. Oops… What is it for then? They proceed to explain that this class was a mistake. Also, it turns out that some system exceptions like TargetInvocationException derive from it.
p. 198: If client code tries to access a nonexistent resource, InvalidOperationException should be thrown. On the other hand, if client code tries to access a resource using an illegal path, ArgumentException should be thrown. The difference looks very subtle to me. Let’s say I let user input a name of a file to read. If the user enters c:tempfoobar.txt and the file is not there, or if the user enters something illegal like a:b:c:temp is the difference really so important? Most programs I know will display “file not found’ in both cases.
p. 199: NullReferenceException, AccessViolationException, or IndexOutOfRangeException. Do argument checking to avoid throwing these exceptions. Throwing these exceptions exposes implementation details… that might change. Well, so what? Argument checking is good and necessary, but beyond that I would not catch and wrap things like IndexOutOfRangeException. If it happened, it happened. Wrapping it will just complicate debugging.
p. 199: Prior to CLR 2.0, there was no distinction between NullReferenceException and AccessViolationException. They then proceed to explain that access violation usually means corrupted memory, while null reference does not. This is a very interesting observation.
p. 202: Do make exceptions serializable. A very, very good point that is frequently forgotten. If not serializable, exception cannot be marshalled across AppDomain boundaries.
p. 203: If a method is supposed to fail often, use either tester-doer or try-parse pattern* – they don’t actually say that in these words. I abbreviated a longer explanation. Jeffrey Richter than proceeds to say, that tester-doer is unreliable in the multi-threaded environment, since test information may become out-of-date by the time you do the do.
Usage Guidelines
p 207: Do prefer using collections over arrays in public APIs. I’d say – if you expose access to your inner fields, use collections with appropriate level of access. If you just copy the data and ship it out, arrays are fine, and easier to use.
p. 217: Do not return null values from collection properties. I am guilty of violating this guideline myself, and I regret that. It is too easy to miss case when the collection is null, and you start getting exceptions. If a property always returns a valid collection, user’s life is much easier.
p. 217: Do not return snapshot collections from properties. It is assumed that properties are lightweight operations. Returning a snapshot means copying the data, which is by definition a heavy operation.
p. 217: Use snapshot or live IEnumerable<T> for collections that are volatile, i.e. can change without [user] explicitly modifying the collection.
p. 220: Avoid implementing collection interfaces on types with complex APIs unrelated to the concept of a collection. In other words, the class should be responsible for one thing, and one thing only. Make collection a property or something.
p. 220: Do use the “Collection” suffix in names of abstractions implementing IEnumerable. This is too strong a guideline for the “DO”. The authors themselves give a couple of counter-examples, such as Set. I would downgrade this to “consider”.
p. 220: … Thus, a collection of IDisposable items can be named DisposableCollection. Hm… Sounds like a bad idea to me. If you ask me what “Disposable collection” might mean, I’d say this is a collection that can be disposed. I would rather use something like ListOfIDisposable. This may look less neat, but at least there is no confusion.
p. 221: Do not implement IClonable. Do not use ICloneable in public APIs. They say that IClonable is bad, because it does not define whether it’s a shallow copy or a deep copy. And it does not define that, partially because it is never used in the framework. The moral of the story is that you should never ship an interface if you dno’t ahve both implementations and consumers of the interface. I searched the Framework sources and could not find even one place where we take
ICloneable as a parameter.
p. 226: Do override GetHashCode if you override Equals.
Do ensure that GetHashCode returns exactly the same value regardless of any changes that are made to the object.
Avoid throwing exceptions from GetHashCode.
This all makes sense only if I want to use my objects as keys in a hash table. If I don’t, it still may make sense to compare the objects for equality. And, of course, mutable objects make bad keys. I would say, that if an object is not intended to be used as a key, GetHashCode should throw a NotSupportedException. This clearly indicates the intent.
p. 227: Do string formatting [in ToString] based on the current thread culture when returning culture depending-information. This is a terrible idea. As they say elsewhere, ToString should not be displayed to a real user. It is for logs, debuggers, and developers. Putting culture-dependent information there is bad. If I get a log that say the date is “3/8/2003” the last thing I want to do is to start guessing whether it is March 8th or August 3rd.
p. 230: Do not use XmlNode or XmlDocument to represent XML data. Favor instances of IXPathNavigable instead. They say XmlDocument and XmlNode tie the API to a particular implementation. I think they should have thought about that when they made XmlDocument and XmlNode concrete classes, and not interfaces like in Java.
To be honest, this the first time I hear about IXPathNavigable. All samples always deal with XmlDocument and XmlNode. I am afraid, many developers will have difficulties working with IXpathNavigable. Besides, is it really a full replacement for XmlDocument? For instance, can I easily write it to a file? I am afraid the answer is no…
I don’t think this is a really bad guideline, but it is an attempt to make developers compensate for questionable decisions made in the framework.
p. 232: Avoid overloading equality operators on mutable reference types. I don’t know… They give a number of arguments, but I am not really convinced. They say not to overload operator== even when I override Equals for mutable reference types. I am afraid this would be very confusing, just as string equality in Java is confusing.