CODEWITHSUNDEEP
c#idisposableusing
Ghostrider
Ghostrider

Reputation: 7775

Getting rid of nested using(...) statements

Sometimes I need to use several disposable objects within a function. Most common case is having StreamReader and StreamWriter but sometimes it's even more than this.

Nested using statements quickly add up and look ugly. To remedy this I've created a small class that collects IDisposable objects and disposes of them when it itself is disposed.

public class MultiDispose : HashSet<IDisposable>, IDisposable
{
    public MultiDispose(params IDisposable[] objectsToDispose)
    {
        foreach (IDisposable d in objectsToDispose)
        {
            this.Add(d);
        }
    }

    public T Add<T>(T obj) where T : IDisposable
    {
        base.Add(obj);
        return obj;
    }

    public void DisposeObject(IDisposable obj)
    {
        obj.Dispose();
        base.Remove(obj);
    }


    #region IDisposable Members

    public void Dispose()
    {
        foreach (IDisposable d in this)
        {
            d.Dispose();
        }

    }

    #endregion
}

So my code now looks like this:

        using (MultiDispose md = new MultiDispose())
        {
            StreamReader rdr = md.Add(new StreamReader(args[0]));
            StreamWriter wrt = md.Add(new StreamWriter(args[1]));
            WhateverElseNeedsDisposing w = md.Add(new WhateverElseNeedsDisposing());

            // code
        }

Is there anything wrong with this approach that can cause problems down the road? I left the Remove function inherited from the HashSet on purpose so that the class would be more flexible. Surely misusing this function can lead to objects not being disposed of properly, but then there many other ways to shoot yourself in the foot without this class.

Upvotes: 27

Views: 9171

Answers (6)

Joe Enos
Joe Enos

Reputation: 40393

I've got to say I disagree with the people who want to do using statements one after the other like:

using (var a = new StreamReader())
using (var b = new StreamWriter())
{
 // Implementation
}

In my opinion, that's very unreadable - having any block of code that's not wrapped is just bad style, and may lead to problems unless all developers working on it are very careful.

I'd put that on par with something like:

if (someBoolean) DoSomething();
{
  // Some code block unrelated to the if statement
}

Technically it's not invalid, but it's awful to look at.

I agree that the MultiDispose concept is probably not the best idea, due to the fact that it's not an accepted pattern, but I definitely wouldn't go this route either. If you can't break things up into smaller chunks, then I'd suggest just living with the nested usings.

Upvotes: 2

Jon Skeet
Jon Skeet

Reputation: 1500695

A few points about the general principle:

  • Your code is distinctly non-idiomatic C#. Basically you're asking anyone else who works with your code to take on board an unusual style for very little benefit.
  • As others have pointed out, you can nest using statements without extra braces
  • If you find yourself with lots of using statements in a single method, you might want to consider breaking it into smaller methods

  • If you have two variables of the same type, you can use a single using statement:

    using (Stream input = File.OpenRead("input.dat"),
       output = File.OpenWrite("output.dat"))
{
}

Now assuming you really want to go ahead with this:

  • Your code will dispose of its contained resources in a hard-to-predict order. Instead of using a set, it should embed a list - and then dispose of things in the reverse order to the calls to Add.
  • There is no reason to derive from HashSet<T> or indeed any collection. You should just have a list within the class as a private member variable.
  • If one of the Dispose calls fails, none of the other Dispose calls will be made; with a traditional using statement, each call to Dispose is made within its own finally block.

Basically, I think it's a bad idea. Nesting two levels deep is far from painful; nesting three should be rare; nesting four or more strongly suggests refactoring. Rather than trying to cope with the pain of deep nesting, you should design away from it.

Upvotes: 21

Josh
Josh

Reputation: 69262

Personally this would drive me nuts. If you are finding nested using statements to be annoying, you could revert to the try/finally syntax. Dispose methods are not supposed to throw exceptions so you could assume that multiple disposables would not need to be individually wrapped in try/finally blocks.

Also worth noting is that you only need one set of brackets for adjacent using blocks like:

using (var stream = File.Open(...))
using (var reader = new StreamReader(stream)) {

   // do stuff

}

Upvotes: 4

SLaks
SLaks

Reputation: 887459

You can make nested using statements prettier by only using one pair of braces, like this:

using (StreamReader rdr = new StreamReader(args[0])) 
using (StreamWriter wrt = new StreamWriter(args[1])) 
{
    ///...
}

To answer your question, you need to dispose in the opposite order of addition.
Therefore, you cannot use a HashSet.

Also, there is no reason to expose the list of IDisposables to the outside world.
Therefore, you should not inherit any collection class, and instead maintain a private List<IDisposable>.

You should then have public Add<T> and Dispose methods (and no other methods), and loop through the list backwards in Dispose.

Upvotes: 7

Chris Taylor
Chris Taylor

Reputation: 53699

Maybe it is just that you have shown a simple example, but I think the following is more readable.

 using (StreamReader rdr = new StreamReader(args[0])) 
 using (StreamWriter wrt = new StreamWriter(args[1])) 
 {     
   // code 
 }

Upvotes: 13

ChaosPandion
ChaosPandion

Reputation: 78272

You could just do this:

using (var a = new A())
using (var b = new B())
{
    /// ...
}

Upvotes: 49

Related Questions