Effective C#

error in Item 5, p. 37

wwagner — Wed, 25 Jul 2007 16:37:45 GMT

Missing format string

On page 37, in item 5, the line of code that reads:

    Console.WriteLine( string.Format( new CustomerFormatter(), "", c1 );

should read:

    Console.WriteLine( string.Format( new CustomerFormatter(), "{0}", c1 );

P 19, (item 3)

wwagner — Sun, 16 Jul 2006 20:47:03 GMT

missing char

The code at the bottom of p. 19 reads:

//Version two

try
{
MyType t1;
t = (MyType)o;

It should be:

try
{
MyType t1;
t1 = (MyType)o;

p. 122 (Item 19)

wwagner — Fri, 07 Jul 2006 23:21:44 GMT

Wrong word in 3rd paragraph

The second sentence in the third paragraph reads: "It [IListSource] also has a containsListCollection property so that users can modify the overall structure of the collection."

It should read: "It [IListSource] also has a containtsListCollection property so that users can navigate the overall structure of the collection."

(Bold indicates the change, not emphasis).

Poor word choice in Item 3

wwagner — Mon, 09 Jan 2006 17:58:39 GMT

You could easily mis-interpret what I said about the as and is operators

At the bottom of page 18, in Item 3, I wrote this sentence:

"If a particular object is not the requested type or is derived from the requested type, they fail."

A better choice would have been:

"If a particular object is not the requested type, nor a type derived from the requested type, they fail."

Errata: Item 10

wwagner — Tue, 15 Nov 2005 08:54:48 GMT

p. 63.

Item 10 incorrectly implies that System.ValueType contains a static operator ==.

It doesn't.

However, many developers do create what seems like a simple solution:

struct MyType
{
// data members elided.
static bool operator == ( MyType l, MyType r )
{
return l.Equals ( r );
}
// Note no override of Equals.
}

That will cause the behavior I mentioned in the item. The reason is that ValueType.Equals() does use reflection to find the values of each field in the objects being compared.

Clarification: Item 22

wwagner — Mon, 29 Aug 2005 18:22:00 GMT

Elided code was not clearly marked
Both listener classes on p. 134 should include a private declaration of the Logger Singleton class:

private static Logger logger;

This declaration was removed from the code listing, but not discussed in the text.

Errata: p. 24

wwagner — Tue, 16 Aug 2005 06:52:26 GMT

2nd paragraph
The sentence "The as operator returns true ..." Should read "The is operator returns true ..."

Errata: p24

wwagner — Thu, 14 Jul 2005 23:59:56 GMT

missing words

The sentence that says, "If you made a create a Newtype" should be "If you made a new class derived from Newtype"

Errata: Missing words on p. 144

wwagner — Fri, 17 Jun 2005 19:49:56 GMT

There really was a full sentence there

p. 144, Item 24, contains the following sentence: "The DefaultSort attribute e, the Name property."

What should be there is this:

"The DefaultSort attribute defines the default sorting property for the Customer class

—in this case, the Name property."

Exceptional conditions about exceptions

wwagner — Thu, 09 Jun 2005 00:11:29 GMT

A number of subtle points about exception processing.

This is one of those lengthy discussions that get filed under “There are always exceptions”. And, it happens to be related to the guidance I wrote on exceptions in Effective C#.

First, the simple question:

“Recently, I and fellow developers have a very heated and passionate debate on whether or not it is appropriate to derive one's application-specific exception from System.ApplicationException or System.Exception. (I was the lone voice, which now advocates the latter despite my past recommendation).
So far Microsoft and all its documentation recommend what you stated in the book and I have been following this recommendation. But of late, particularly in Whidbey Beta 1, the recommendation has changed. Even FxCop 1.312 for Whidbey has changed.
It now recommends the application-specific exception *should* derive from System.Exception instead.
I was initially furious with FxCop but after doing research, particularly SLAR (.Net Framework Standard Library Annotated Reference) and this URL go further to suggest the BCL team has admission of mistake: http://blogs.msdn.com/brada/archive/2004/03/25/96251.aspx”

Well, yeah, we all goofed. It turns out that creating an application specific base class for exceptions doesn’t buy anything. In fact, it only makes writing good catch clauses harder. As I pointed out in Item 44, the main reason to create new exception clauses is to facilitate catching different errors and responding to them with different actions. Knowing that an exception was generated in application code vs. library code doesn’t really play into that decision. In fact, all it does is introduce an extra level of complexity in the class hierarchy. The world is a simpler place without using ApplicationException.

And now, the subtle correct behavior in Item 44:

“Also because Exception is derived from ISerializable, I don't seem to recall seeing the mentioning of ISerializable.GetObjectData(), which is mentioned in Item 25).”

Correct, I did not add a GetObjectData() method in any of the examples in Item 44. The reason is simple: None of these classes add any data fields not already found in the base class, System.Exception. For that reason the correct implementation of any derived GetObjectData would do nothing except call the base class, providing no extra functionality. I should have explained my reason for not including it, but the samples are correct without it.

And finally, the tough one:

“In relation to the use of Inner Exception, I have two minds about it. One is if you provide a form of abstraction where the client of your class/assembly needs not be aware of the implementation details. If the class designer wisely implements an Application-specific exception in the public interface (which exceptions are part of it) to support exception translation.

If the class designer unwittingly uses inner exception to embed the exception thrown by the implementation assembly, then this forces the client also to reference the implementation assembly in order to deserialize the application-specific exception. This to me is a break in abstraction. The problem is particularly acute in cross app domain calls, such as remoting. For instance class that support remoting service from a Data Access Layer. The client should not need to include say SqlClient in order to deserialize an exception thrown from the DAL.

The other area where the embedded inner exception can hurt is in those assembly that are loaded dynamically via Assembly.Load() using configuration details, generally in their own app domain. When the exception is deserialized it would also need to assembly supporting the inner exception to be available on the receiving side. Failure to have them can result in a deserialization exception.

In this usage, I tend to recommend to my fellow developers to avoid embedding exception. But rather constructed an exception message that include enough details of the implementation detail for technical staff to perform diagnostic.”

This is the difficult question, because it touches on many edge cases. First, let’s cover the normal case: You should fill in the InnerException property, because it’s how you preserve as much information as possible about the original error. Failure to do so means that the technical staff loses that information.

Now let’s look at the two cases my reader mentioned. First, let’s cover remoting. Ideally, you’d like to preserve the InnerException property. But, it’s just not safe. As the reader points out, if the implementation of the runtime class used in the InnerException isn’t available on the client, the client gets a SerializationException. That effectively loses all information in your original exception, so it doesn’t help anyone.

You might be tempted to look at the runtime type of the exception and include it as the InnerException property if it is delivered with the framework, and therefore available on the client machine. That’s a mistake, because you would need to walk all the InnerException property of each inner exception to ensure that you never find a type that might not be delivered to the client.

The Assembly.Load() gives similar results but for different reasons. Once again, you don’t know what domain into which you code will be loaded. If an exception type (or any other type, for that matter), gets passed across the domain boundary, your client process must catch serialization exceptions.

The root cause in both cases is that an object is being passed across process boundaries. As an aside, you can create the same problems by returning a derived class where a base class was specified. If the derived class implementation is not available to the client process, it cannot be serialized, and the client must catch serialization exceptions.

So, what should you do? Well, this is one of the reasons I prefer an SOA model, or web services implementation, to communicate between processes. The Xml Serializer is more resilient in these kinds of situations. In fact, you can’t really throw an exception from a web service to the client application. Instead, you return an error document.

If you have chosen remoting and you’re planning to stick with it, you just have to remember that the app domain boundary is always a special case, and all the concrete types that are passed through that boundary must be available on both sides of the boundary. As the reader mentions, you need to use some other technique to preserve exception information. My own preference would be to log all information about the exception on the server, using something like the Exception Processing block in the Enterprise Library. After that’s done, you can create a new exception that references the logged information and sends that to the client for client side processing and reporting.

Inter-process (and app domain) boundaries are edge cases, and you need to take special care with information that crosses those boundaries, including exceptions.

.NET Framework changes affecting Items 9, 10

wwagner — Wed, 08 Jun 2005 15:53:03 GMT

The advice is still proper, but the justification is somewhat different.

The 2.0 version of the .NET framework has changed the way they implement ValueType.Equals() and ValueType.GetHashCode(). This affects some of the justification for my advice in items 9 and 10.

I explained in Item 9 that these methods use Reflection to determine if the values stored are equal. Further, that as a performance improvement, only the first field in the value type was compared, instead of each field. A similar algorithm was used in ValueType.GetHashCode(): This method returned the hash code of the first field in the value type.

This behavior changes in 2.0. Beta 1 and Beta 2 now use all the fields in a value type to determine equality, and to calculate the hash code.

These changes do not fundamentally affect the advice I set forth in Items 9 and 10. (In fact, if you are already following my advice, the changes to the implementation of these methods will be transparent to you.) You should still override both methods, Equals and GetHashCode) for ValueTypes. While the implementations in the 2.0 framework are more correct than their predecessors, they will have a negative impact on performance because of their use of reflection. Furthermore, by creating your own implementation, you shield yourself against this type of version-to-version implementation changes.

Question on Array Recommendations

wwagner — Mon, 28 Feb 2005 16:29:16 GMT

They are slightly different than those from the P & P Guide.

Question on Array Recommendations.

I finished your book a few days ago. It was a fun read, and I think you covered a lot of good areas for code improvement. I found one of your recommendations that contradicts with something I found in the Patterns and Practices Performance and Scalability book from Microsoft dealing with Multidimensional Arrays vs Jagged Arrays. You seem to be in favor of the Multidimensional Arrays over Jagged (Item 40, page 231-232). Here's what the P&P book has to say on that:

Chapter 5, page 241-242:

"Use Jagged Arrays Instead of Multidimensional Arrays

A jagged array is a single dimensional array of arrays. The elements of a jagged array can be of different dimensions and sizes. Use jagged arrays instead of multidimensional arrays to benefit from MSIL performance optimizations. MSIL has specific instructions that target single dimensional zero-based arrays (SZArrays) and access to this type of array is optimized. In contrast, multidimensional arrays are accessed using the same generic code for all types, which results in boxing and unboxing for arrays of primitive types."

They then get into some MSIL code comparing the two.

My Response:

This is one of those low-level performance issues where the performance metrics depend on the particular program. I'll get to the performance characteristics in a moment.

My overriding reason for recommending multi-dimensional arrays stems from ease of use, and maintenance. A multi-dimensional array is a single structure. Simulating multiple dimensions using jagged arrays is more complex: You create one array per row, plus the outer array. Quite simply, it's easier to get wrong, resulting in null reference exceptions.

This added complexity manifests itself in many ways. First, you write more initialization code (shown on p. 230-231 of Effective C#). Iterating all the members of a jagged array is more complex as well. A single for or foreach loop gets replaced with nested loops. I discuss this on p. 232-233.

While this may be simple code when you intend to model a multi-dimensional array using jagged arrays, there is no guarantee that all rows have the same number of elements. This also complicated column-wise traversals: You should check that each row has enough elements before simply assuming it works. If you want to support a single enumerator for a jagged array, you must write your own. It's already available in a multi-dimensional array.

These differences make me lean toward simplicity rather than speed. Quite frankly, a little slower but more maintainable code will scale better than overly optimized but harder to maintain code. That means I prefer the multi-dimensional array.

Finally, I'll address the performance issues. It depends on what you measure, and where your programs bottlenecks are:

A multi-dimensional array is one allocation; a jagged array causes N+1 allocations, where N is the number of rows. That has some cost.

Row-wise traversals will be faster for jagged arrays, as pointed out in the P&P guide.

Column-wise traversals will be faster for multidimensional arrays, as I point out in p. 231.

In all three cases, the differences will be small. You will only see the difference for an operation that is repeated very often. Before you make this kind of a change, you should profile code and determine which version is fastest. And, you should know that the speed difference is significant enough to justify the change.

Questions and answers about events and threads

wwagner — Mon, 14 Feb 2005 08:22:16 GMT

This pertains to Item 22, and Item 45.

Question 1:

Can you further explain the idiom you recommend on top of page 133 (see below) to raise an event. Specifically, how does making a copy of the reference provide protection?

// add a message, and log it.
public void AddMsg ( int priority, string msg )
{
// This idiom discussed below.
AddMessageEventHandler l = Log;
if ( l != null )
l ( null, new LoggerEventArgs( priority, msg ) );
}

In my mind a "copy of a reference" is, well, just a copy-of-a-reference, which would not prevent the referenced object from being changed elsewhere. I would agree that a copy of the underlying _object_ would provide protection, but that doesn't seem to be what's going on here.

Answer 1:

This idiom does work because of the behavior of the Add and Remove accessors for an event. If another thread calls Remove(), that does not modify the invocation list you’ve copied to the local variable ‘l’. Rather, it creates a new invocation list with no targets. Your invocation list is still valid. You are not making a deep copy of the invocation list, but any other thread the modifies your invocation list does.

Question 2:

Also while on this topic, do you recommend protecting the actual event call with a Try/Catch as a way to protect against exception-throwing event handlers (that others might write, of course)? Not sure that there would be anything to do in the Catch, maybe just Catch {}. Interested to know what you think.

Answer 2:

It varies. I believe that event handlers should never throw exceptions. (See Item 45, p. 270). However, I don’t believe that you can always count on others to follow that rule. If you are creating libraries that will be used by a large audience, you should be very defensive and wrap event handler invocations with try / catch. When you are creating closed systems, and you can ensure that none of the event handlers could possibly throw exceptions, I find that the simpler syntax easier to read and maintain.

P. 111, Item 17

wwagner — Fri, 21 Jan 2005 02:09:33 GMT

Implement the Standard Dispose Idiom

At the bottom of the page, the line:

GC.SuppressFinalization( true );

should be:

GC.SuppressFinalize( this );

Found by Barrie Green.

Question on Item 4

wwagner — Tue, 11 Jan 2005 20:56:33 GMT

MSDN uses a different idiom to support multiple conditions on an attribute. I think it has a number of shortcomings.

Question: Marc Jennings writes:

In the section on Conditionals you say that in order to create a method that will fire if more than one condition is true (logical AND), you need to use

#if (VAR1 && VAR2)
#define BOTH
#endif

However, it is possible to chain the conditionals together (according to MSDN, anyway…)

[Conditional("A")] public static void IfAandB( )
{
AandBPrivate( );
}

[Conditional("B")] static void AandBPrivate( )
{
/* Code to execute when both A and B are defined... */
}

Call IfAandB; if both A and B are defined, AandBPrivate will execute.

</QUOTE>

Is there any reason to use your method above the MS one?

Answer:

I chose using the #if logic over chaining conditionals because it produces code that is much easier to maintain.

Using the idiom I preferred, the MSDN sample would look like this:

#if (A && B)
#define BOTH
#endif

[Conditional("BOTH")] static void AandBPrivate( )
{
/* Code to execute when both A and B are defined... */
}
I look at the top of the file to find any defined environment variables. And, the single environment variable used to control the compilation of a function is found on the method in question. The sample from MSDN can be perverted in a couple ways. First, I can easily circumvent the first check:

/* No conditional attribute */ static void BPrivate( )
{
AandBPrivate( );
}

The code inside AandBPrivate() can be accessed when only B is defined. That certainly looks like it was not intended.

The MSDN idiom does not scale well either. Putting the Boolean logic in the conditional, I can easily create more complicated expressions:

#if (A && B && C && D)
#define ALL
#endif

To do the same thing following the MSDN idiom requires this:

[Conditional("A")] public static void IfAandB( )
{
   AandBPrivate( );
}

[Conditional("B")] static void AandBPrivate( )
{
   AandBAndCPrivate( );
}

[Conditional("C")] static void AandBandCPrivate( )
{
   AandBAndCandDPrivate( );
}

[Conditional("D")] static void AandBandCandDPrivate( )
{
   AandBAndCandDPrivate( );
}

It gets maddening fairly quickly.

Finally, putting the conditionals at the top of the file means I can add methods that are dependent on the absence of an environment variable, which is not supported by the conditional attribute:

#if ( ! D )
#define NotD
#endif

[Conditional("NotD")] static void NotDPrivate( )
{
/* contents elided. */
}