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.

score 10 · Answer 1 · answered Jun 11 '10 at 19:50

10

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. :)

answered Jun 11 '10 at 19:50

tzaman

46,925
11
90
115

That wont throw a null reference exception when it tries to call the extension method? Edit-- I wrote a quick test app, and it works great! – Scott Chamberlain Jun 11 '10 at 19:55
No, it won't. I used a similar approach in a project (DisposeIfNotNull()). – Eric Olsson Jun 11 '10 at 19:59
2

Nope! Since the dot operator is just syntactic sugar for a static method, it works just fine. :) – tzaman Jun 11 '10 at 20:10
This is one of the things i liked the most about extension methods. Unfortunately in my current project we ar working on 2.0, so no choice of extension methods. – Manish Basantani Jun 12 '10 at 14:04

Dan Tao · Accepted Answer · 2012-07-09T18:03:58.090

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);
    }
}

As an added bonus, this also resolves a potential race condition in your original code. If running in a multithreaded context, the if (_field != null) _field.Dispose() pattern can result in a NullReferenceException when _field is set to null between the check and the disposal (rare, but possible). Passing _field as an argument to a method such as DisposeMember copies the reference to a local variable in the method, eliminating this possibility, unlikely as it is.

Agreed with it not being a design flaw. Maybe I'm overly cautious, but I prefer to have a null-check even when I certifiably know that the object will never be null when it comes to disposables. — Grace Note, Jun 11 '10 at 12:38
@ccomet: +1, i have the same thinking (overly cautious) at the back of mind while calling Dispose() on an object. — Manish Basantani, Jun 11 '10 at 12:43

score 0 · Answer 3 · answered Jun 11 '10 at 12:35

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.)

score 0 · Answer 4 · answered Jun 11 '10 at 12:37

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.

score -1 · Answer 5 · answered Jun 11 '10 at 12:38

-1

Try this.

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

answered Jun 11 '10 at 12:38

this. __curious_geek

42,787
22
113
137

I'm not sure I understand what you're suggesting here. – Dan Tao Jun 11 '10 at 12:41
4

It wasn't me, but checking whether the member is a `IDisposable` is a bit silly, because a call to `member.Dispose()` would not compile when `member` was not a disposable type. – Steven Jun 11 '10 at 13:35
http://stackoverflow.com/questions/2349378/new-programming-jargon-you-coined/2430307#2430307 P.S. I did't downvote, but null != member just reminded me on that post ;) – Jürgen Steinblock Jun 11 '10 at 20:03

Disposing the members that implement IDisposable

5 Answers5

Linked