The Black Box of .NET Headline Animator

The Black Box of .NET
Showing posts with label best practices. Show all posts
Showing posts with label best practices. Show all posts

Friday, May 18, 2012

Why you should use ReadOnlyCollection<T>

Many people believe that you should be using List<T> to return collections from a public API.  However, there are several reasons not to do so; instead return a ReadOnlyCollection<T>. The are several benefits of using a ReadOnlyCollection<T>:
  1. The *collection itself* cannot be modified – not the items within it. i.e. you cannot add, remove, or replace any item in the collection. So, it keeps the collection itself intact – not the values of the items within it. To protect the items in it, you’d probably have to have private “setters” on any properties for the reference object in the collection – which is generally recommended. However, items that are declared as [DataMember] must have both public getters and setters. So if you are in that case, you can’t do the private setter, but you can still use the ReadOnlyCollection<T>. Using this as a Design paradigm can prevent subtle bugs from popping up.
  2. Performance: The List<T> class is essentially a wrapper around a strongly-typed array. Since arrays are immutable (cannot be re-sized), any modifications made to a List<T> must create a complete copy of List<T> and add the new item. The initial capacity for a List<T> is ‘4’ unless specified thru the overloaded constructor. Obviously this can be very bad for memory and performance. Don’t forget that not only the items directly within the List<T> are copied – but every value and object with the entire object graph for that reference type – this could easily contain other references type, other collections, etc. This is why it is best practice to create/initialize a List<T> instance with the overloaded constructor which takes an ‘int’ denoting the size of the List to be created. This can almost always be done since you are usually iterating on a “known” size value at runtime. For example, creating a List<T> of objects from a "Repository or DataService" method usually iterates/reads from a IDataReader object which has a property of ‘RecordsAffected’. If you are going to be putting an item in the List<T> based on the number of times the IDataReader is Read – while(reader.Read()) you can easily create the list like so (and it is preferable to do it like this):
    if (reader != null && reader.RecordsAffected > 0)
    {
        List<Foo> someList = new List<Foo>(reader.RecordsAffected);
        while (reader.Read())
        {
            someList.Add(...);
            ...
        }
    }

Just as a side note…every time that a List<T> needs to grow in size, the size is *doubled*. So, if you happen to add just one more item to a List<T> that wasn’t constructed with a pre-determined size, and the List<T> expands to accommodate the new item, there will be a whole lot of unused space at the end of the List<T> even though there aren’t any items in it – bad for memory and performance.
Share

Thursday, May 17, 2012

Don't implement GetHashCode() on mutable fields/properties

CodeProject You shouldn't ever implement GetHashCode on mutable properties (properties that could be changed by someone) - i.e. non-private setters.   I've seen this done in several places and it results in very difficult to find bugs.

Here's why - imagine this scenario:
  1. You put an instance of your object in a collection which uses GetHashCode() "under the covers" or directly (Hashtable).
  2. Then someone changes the value of the field/property that you've used in your GetHashCode() implementation.
Guess what...your object is permanently lost in the collection since the collection uses GetHashCode() to find it! You've effectively changed the hashcode value from what was originally placed in the collection. Probably not what you wanted.
Share

Tuesday, March 13, 2012

Don't use 'using()' with a WCF proxy


If you're trying to be a conscientious developer and making sure that you cleanup your resources - great! You are writing 'using()' blocks around all of your disposable items - great...except when the disposable item is a WCF Client/Proxy! The using() statement and the try/finally effectively have the same IL:

    // The IL for this block is effectively the same as
    // the IL for the second block below
    using (var win = new Form())
    {
    }

   
    // This is the second block
    Form f = null;
    try
    {
        f = new Form();
    }
    finally
    {
        if (f != null)
        {
            f.Dispose();
        }
    }

Here's the IL for the 'using()' block above compiled in Release mode:

     IL_0000:  newobj     instance void [System.Windows.Forms]System.Windows.Forms.Form::.ctor()
     IL_0005:  stloc.0
     .try
     {
         IL_0006:  leave.s    IL_0012
     }  // end .try
     finally
     {
         IL_0008:  ldloc.0
         IL_0009:  brfalse.s  IL_0011
         IL_000b:  ldloc.0
         IL_000c:  callvirt   instance void [mscorlib]System.IDisposable::Dispose()
         IL_0011:  endfinally
     }  // end handler


Here's the IL for the second block (try/finally) compiled in Release mode:

     IL_0012:  ldnull
     IL_0013:  stloc.1
     .try
     {
         IL_0014:  newobj     instance void [System.Windows.Forms]System.Windows.Forms.Form::.ctor()
         IL_0019:  stloc.1
         IL_001a:  leave.s    IL_0026
     }  // end .try
     finally
     {
         IL_001c:  ldloc.1
         IL_001d:  brfalse.s  IL_0025
         IL_001f:  ldloc.1
         IL_0020:  callvirt   instance void [System]System.ComponentModel.Component::Dispose()
         IL_0025:  endfinally
     }  // end handler

As you can see, the IL is nearly identical.

Well this is all fine and good but let's get back to the issue with WCF.  The problem is that if an exception is thrown during disposal of the WCF client/proxy, the channel is never closed.  Now, in general, any exception that occurs during disposal of an object is indeed undesirable.  But, in the case of WCF, multiple channels remaining open could easily cause your entire service to fall on its face - not to mention what might eventually happen to your web server.

Here is an alternative solution that can be used:

    WCFProxy variableName = null;
    try
    {
        variableName = new WCFProxy();

        // TODO code here

        variableName.Close();
    }
// if you need to catch other exceptions, do so here...
    catch (Exception)
    {
        if (variableName != null)
        {
            variableName.Abort();
        }
        throw;
    }

MSDN does have a brief on this issue which you can read here - http://msdn.microsoft.com/en-us/library/aa355056.aspx

Share

Thursday, January 26, 2012

Why you should comment your code...

Here are the most important reasons for including comments/documentation when writing code:
  1. I absolutely believe in documenting code using both XML doc comments on methods as well as unlined comments when/where necessary. This facilitates docentation files being generated automatically and can be used to create .chm files. For all of us who are lazy or those that think it takes too much time, use GhostDoc. Thus, laziness is no excuse.
  2. What if you have to maintain code that was written without comments? Do you want to waste your time digging thru code? What if it's more than just a method or class? What if its a library or framework? I personally have better things to do with my time.
  3. What about someone new to your team? What if your the new guy? Is it easier to learn it with or without documentation/comments?
  4. If you are writing a framework, library or API that will be used by other teams/API consumers, how are they supposed to know what it does without documentation? Do you expect them to dig thru your code to figure it out? What if they don't have access to the code? Would you want to have to do this?
  5. What if you find some code without comments and the code looks wrong or inefficient? It's certainly possible that it was written that way for a reason. Only comments would help.
  6. In addition to adding comments, code itself should be self documenting: use descriptive class, member, variable, parameter and method names.
When you write code, remember that you're not always going to be the one modifying, supporting it or consuming it...

Peter Ritchie has a good post about what comments are NOT for - a pretty good read: http://msmvps.com/blogs/peterritchie/archive/2012/01/30/what-code-comments-are-not-for.aspx


Share

Wednesday, March 2, 2011

How do you properly implement the IDisposable pattern?

This is the first post of a series that I'll be making on memory management in .NET.  The subsequent posts will cover guidelines for working with disposable objects, when to implement IDisposable, and maybe a few others.

I am often asked how to correctly implement the IDisposable pattern. There are many posts you'll see from a Google search that try to answer this question. IMHO, the pattern that you find here is the safest and most correct implementation of the pattern according to .NET Best Practices and my experience. I am confident saying this because of my extensive experience with memory leak debugging, internals of the CLR, code reviews, and implementation of best practices at very large, well-known clients who rely on solid applications.  You will find that my provided implementation of this pattern is slightly different than that which is posted on MSDN.  IMHO, it isn't sufficient.  You have to agree that a large number of samples and examples posted on MSDN are "hacky" and don't do things the way that they should be done in the "real world".

A couple of notes on implementing the IDisposable pattern:
  1. Simply adding a Dispose() method to a class doesn't implement the pattern (just because it compiles doesn't mean it's correct).  I've seen this mistake made many, many times - it's just not a good idea.
  2. Adding the code below isn't enough - you must also explicitly declare the class inheriting from IDisposable ->  MyDisposableClass : IDisposable.  This is very important since objects creating/managing instances of your class might decide to try to cast it to IDisposable and call Dispose or use a 'using()' statement. If your class isn't explicitly declared to implement IDisposable, the cast will fail and Dispose will never be called.
  3. Just because you implement IDisposable correctly doesn't mean it will get called "magically". What I mean is that the GC never calls Dispose for you - you have to explicitly call it on your class from the owning object.  There is a common misconception that this can somehow magically happen by the GC and that it's not important that you explicitly call it "because the GC cleans up for you" (WRONG!)
  4. Implementing the IDisposable pattern correctly is tricky and can easily lead to problems if not done properly.  This is why I've provided the code below.
  5. I use a code snippet with the exact code below to properly and consistently implement the pattern correctly.
  6. There is great debate on whether or not you should null out rooted references. It certainly never hurts to do so. Nulling these out does do something before GC runs - it removes the rooted reference to that object. The GC later scans its collection of rooted references and collects those that do not have a rooted reference.

    Think of this example when it is good to do so: you have an instance of type "ClassA" - let's call it 'X'. X contains an object of type "ClassB" - let's call this 'Y'. Y implements IDisposable, thus, X should do the same to dispose of Y. Let's assume that X is in Generation 2 or the LOH and Y is in Generation 0 or 1. When Dispose() is called on X AND that implementation nulls out the reference to Y, the rooted reference to Y is immediately removed. If a GC happens for Gen 0 or Gen 1, the memory/resources for Y is cleaned up but the memory/resources for X is not since X lives in Gen 2 or the LOH.
Here is the code for properly implementing the IDisposable pattern in a base class:


#region IDisposable base-class implementation
 
//TODO remember to make this class inherit from IDisposable -> MyDisposableClass : IDisposable
 
/// <summary>
/// Gets or sets a value indicating whether this instance is disposed.
/// </summary>
/// <value>
///  <c>true</c> if this instance is disposed; otherwise, <c>false</c>.
/// </value>
/// <remarks>Default initialization for a bool is 'false'</remarks>
private bool IsDisposed { getset; }
 
/// <summary>
/// Implementation of Dispose according to .NET Framework Design Guidelines.
/// </summary>
/// <remarks>Do not make this method virtual.
/// A derived class should not be able to override this method.
/// </remarks>
public void Dispose()
{
    Dispose(true);
 
    // This object will be cleaned up by the Dispose method.
    // Therefore, you should call GC.SupressFinalize to
    // take this object off the finalization queue 
    // and prevent finalization code for this object
    // from executing a second time.
 
    // Always use SuppressFinalize() in case a subclass
    // of this type implements a finalizer.
    GC.SuppressFinalize(this);
}
 
/// <summary>
/// Overloaded Implementation of Dispose.
/// </summary>
/// <param name="isDisposing"><c>true</c> to release both managed and unmanaged resources; 
/// <c>false</c> to release only unmanaged resources.</param>
/// <remarks>
/// <list type="bulleted">Dispose(bool isDisposing) executes in two distinct scenarios.
/// <item>If <paramref name="isDisposing"/> equals true, the method has been called directly
/// or indirectly by a user's code. Managed and unmanaged resources
/// can be disposed.</item>
/// <item>If <paramref name="isDisposing"/> equals <c>false</c>, the method has been called 
/// by the runtime from inside the finalizer and you should not reference
/// other objects. Only unmanaged resources can be disposed.</item></list>
/// </remarks>
protected virtual void Dispose(bool isDisposing)
{
    // TODO If you need thread safety, use a lock around these 
    // operations, as well as in your methods that use the resource.
    try
    {
        if (!this.IsDisposed)
        {
            // Explicitly set root references to null to expressly tell the GarbageCollector
            // that the resources have been disposed of and its ok to release the memory 
            // allocated for them.
            if (isDisposing)
            {
                // Release all managed resources here


                // Need to unregister/detach yourself from the events. Always make sure
                // the object is not null first before trying to unregister/detach them!
                // Failure to unregister can be a BIG source of memory leaks
                if (someDisposableObjectWithAnEventHandler != null)
                {                 
                    someDisposableObjectWithAnEventHandler.SomeEvent -= someDelegate;
                    someDisposableObjectWithAnEventHandler.Dispose();
                    someDisposableObjectWithAnEventHandler = null;
                }


                // If this is a WinForm/UI control, uncomment this code
                //if (components != null)
                //{
                //    components.Dispose();
                //}
            }
            // Release all unmanaged resources here  
            // (example)             if (someComObject != null && Marshal.IsComObject(someComObject))
            {
                Marshal.FinalReleaseComObject(someComObject);
                someComObject = null;
            }
        }
    }
    finally
    {
        this.IsDisposed = true;
    }
}

//TODO Uncomment this code if this class will contain members which are UNmanaged
///// <summary>Finalizer for MyDisposableClass</summary>
///// <remarks>This finalizer will run only if the Dispose method does not get called.
///// It gives your base class the opportunity to finalize.
///// DO NOT provide finalizers in types derived from this class.
///// All code executed within a Finalizer MUST be thread-safe!</remarks>
//  ~MyDisposableClass()
//  {
//     Dispose( false );
//  }
#endregion IDisposable base-class implementation
 
Here is the code for properly implementing the IDisposable pattern in a derived class:

#region IDisposable derived-class implementation

/// <summary>
/// Gets or sets a value indicating whether this instance is disposed.
/// </summary>
/// <value>
///  <c>true</c> if this instance is disposed; otherwise, <c>false</c>.
/// </value>
/// <remarks>Default initialization for a bool is 'false'</remarks>
private bool IsDisposed { getset; }

/// <summary>
/// Overloaded Implementation of Dispose.
/// </summary>
/// <param name="isDisposing"><c>true</c> to release both managed and unmanaged resources; 
/// <c>false</c> to release only unmanaged resources.</param>
/// <remarks>
/// <list type="bulleted">Dispose(bool isDisposing) executes in two distinct scenarios.
/// <item>If <paramref name="isDisposing"/> equals true, the method has been called directly
/// or indirectly by a user's code. Managed and unmanaged resources
/// can be disposed.</item>
/// <item>If <paramref name="isDisposing"/> equals <c>false</c>, the method has been called

/// by the runtime from inside the finalizer and you should not reference
/// other objects. Only unmanaged resources can be disposed.</item></list>
/// </remarks>
protected override void Dispose(bool isDisposing)
{
    // TODO If you need thread safety, use a lock around these 
    // operations, as well as in your methods that use the resource.
    try
    {
        if (!this.IsDisposed)
        {
            // Explicitly set root references to null to expressly tell the GarbageCollector
            // that the resources have been disposed of and its ok to release the memory 
            // allocated for them.
            if (isDisposing)
            {
                // Release all managed resources here

                // Need to unregister/detach yourself from the events. Always make sure
                // the object is not null first before trying to unregister/detach them!
                // Failure to unregister can be a BIG source of memory leaks
                if (someDisposableObjectWithAnEventHandler != null)
                {                 
                    someDisposableObjectWithAnEventHandler.SomeEvent -= someDelegate;
                    someDisposableObjectWithAnEventHandler.Dispose();
                    someDisposableObjectWithAnEventHandler = null;
                }
                // If this is a WinForm/UI contrlol, uncomment this code
                //if (components != null)
                //{
                //    components.Dispose();
                //}
            }

            // Release all unmanaged resources here
 
 
            // (example)
            if (someComObject != null && Marshal.IsComObject(someComObject))
            {
                Marshal.FinalReleaseComObject(someComObject);
                someComObject = null;
            }
        }
    }
    finally
    {
        this.IsDisposed = true;

        // explicitly call the base class Dispose implementation
        base.Dispose(isDisposing);

    }
}
#endregion IDisposable derived-class implementation


Share