Disposing the members that implement IDisposable.

Question

In my Dispose methods (like the one below), everytime i want to call someObj.Dispose() i also have a check for someObj!=null.

Is that because of bad design on my part? Is their a cleaner way to ascertain that Dispose of all the members (implementing IDisposable) being used in an object is called without having a risk of NullReference exception ?

protected void Dispose(bool disposing)
        {
            if (disposing)
            {
               if (_splitTradePopupManager != null)
                {
                    _splitTradePopupManager.Dispose();
                }
             }
        }

Thanks for your interest.

Answer 1

Only you know the answer to this one!

Without seeing your entire class it's difficult for anyone else to tell if it's possible that those members will ever be null when Dispose is called.

(Of course, as a general rule it's always possible for a reference type or nullable value type to be null, so it's probably good practice to always include those null checks anyway.)

Answer 2

When you reference an instance to the _splitTradePopupManager field during object initializition (constructor) and never nullify this instance, you won't have to check for null. In all other situations, you'd better check for null, as you are already doing, because throwing a NullReferenceException (or any exception for that matter) from a dispose method is a bad thing.

In other words, I would say your design is good.

Answer 3

Maybe someone else can chime in on this, but I don't personally think it's a design flaw -- just the safest way to do it.

That said, nothing's stopping you from wrapping your null check and Dispose call in a convenient method:

private void DisposeMember(IDisposable member)
{
    if (member != null)
        member.Dispose();
}

Then your Dispose method could look a bit cleaner:

protected void Dispose(bool disposing)
{
    if (disposing)
    {
        DisposeMember(_splitTradePopupManager);
        DisposeMember(_disposableMember2);
        DisposeMember(_disposableMember3);
    }
}

Answer 4

The only other option I could think of would be to create a DisposeParameter helper method that has an object as it's parameter and only checks if it's null and otherwise Dispose it. That way you'd only need one line of code to dispose of it, but I'm not sure if it would make it more readable.

Answer 5

Try this.

    protected void Dispose(bool disposing) 
    { 
        if (disposing) 
        {
           //for all members.. 
           if (null != member && member is IDisposible) 
            { 
                member.Dispose(); 
            } 
         } 
    } 

Answer 6

I like @Dan Tao's solution, but it's much better as an extension method, imo:

public static void SafeDispose(this IDisposable obj)
{
    if (obj != null)
        obj.Dispose();
}

Now you can just call member.SafeDispose() on any IDisposable in your program without worry. :)