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.
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.
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. :)
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.)
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.
Try this.
protected void Dispose(bool disposing)
{
if (disposing)
{
//for all members..
if (null != member && member is IDisposible)
{
member.Dispose();
}
}
}