How could one refactor code involved in nested usings?

Question

I've got some code that has an awful lot of duplication. The problem comes from the fact that I'm dealing with nested IDisposable types. Today I have something that looks like:

public void UpdateFromXml(Guid innerId, XDocument someXml)
{
    using (var a = SomeFactory.GetA(_uri))
    using (var b = a.GetB(_id))
    using (var c = b.GetC(innerId))
    {
        var cWrapper = new SomeWrapper(c);
        cWrapper.Update(someXml);
    }
}

public bool GetSomeValueById(Guid innerId)
{
    using (var a = SomeFactory.GetA(_uri))
    using (var b = a.GetB(_id))
    using (var c = b.GetC(innerId))
    {
        return c.GetSomeValue();
    }
}

The whole nested using block is the same for each of these methods (two are shown, but there are about ten of them). The only thing that is different is what happens when you get to the inner level of the using blocks.

One way I was thinking would be to do something along the lines of:

public void UpdateFromXml(Guid innerId, XDocument someXml)
{
    ActOnC(innerId, c =>
    { 
        var cWrapper = new SomeWrapper(c);
        cWrapper.Update(someXml);
    });
}

public bool GetSomeValueById(Guid innerId)
{
    var result = null;

    ActOnC(innerId, c => { result = c.GetSomeValue(); });

    return result;
}

private void ActOnC(Guid innerId, Action<TheCType> action)
{
    using (var a = SomeFactory.GetA(_uri))
    using (var b = a.GetB(_id))
    using (var c = b.GetC(innerId))
    {
        action(c);
    }        
}

This works, it's just kind of clunky to parse (as a human). Does anyone have any other suggestions on how one could reduce the code duplication around nested using blocks like this? If they weren't IDisposable then one would likely just create a method to return the results of b.GetC(innerId) ... but that isn't the case here.

+1 I do not see anything clunky in your solution. It is kind of unorthodox being more functional than procedural, but I would count it as a pro, not a con — mfeingold, Apr 23 '12 at 17:49
I think your implementation looks fine but maybe you will prefer some of the alternatives provided below. If you find yourself having to chain lots of disposables you may want look at redesigning things so you don't end up in this situation. — Thomas, Apr 23 '12 at 17:58

score 1 · Answer 1 · answered Apr 23 '12 at 17:52

In the Rx framework there's a class called CompositeDisposable http://msdn.microsoft.com/en-us/library/system.reactive.disposables.compositedisposable%28v=vs.103%29.aspx

Shouldn't be too hard to roll your own (albeit very stripped down version):

public class CompositeDisposable : IDisposable
{
    private IDisposable[] _disposables;

    public CompositeDisposable(params IDisposable[] disposables)
    {
        _disposables = disposables;
    }

    public void Dispose()
    {
        if(_disposables == null)
        {
            return;
        }

        foreach(var disposable in _disposables)
        {
            disposable.Dispose();
        }
    }
}

Then this looks a little cleaner:

public void UpdateFromXml(Guid innerId, XDocument someXml)
{
    var a = SomeFactory.GetA(_uri);
    var b = a.GetB(_id);
    var c = b.GetC(innerId);
    using(new CompositeDisposable(a,b,c))
    {
        var cWrapper = new SomeWrapper(c);
        cWrapper.Update(someXml);
    }
}

what if an exception occurs during b.GetC - I don't think a and b are disposed properly when that occurs. — Amy B, Apr 23 '12 at 18:16

score 1 · Accepted Answer · edited May 23 '17 at 10:24

I like the answer provided by BFree as a start, but I'd make a few modifications.

//Give it a better name; this isn't designed to be a general purpose class
public class MyCompositeDisposable : IDisposable 
{
    public MyCompositeDisposable (string uri, int id, int innerid)
    {
        A = SomeFactory.GetA(uri);
        B = A.GetB(id);
        C = B.GetC(innerId);
    }

    //You can make A & B private if appropriate; 
    //not sure if all three or just C should be exposed publicly.
    //Class names are made up; you'll need to fix.  
    //They should also probably be given more meaningful names.
    public ClassA A{get;private set;}
    public ClassB B{get;private set;}
    public ClassC C{get;private set;}

    public void Dispose()
    {
        A.Dispose();
        B.Dispose();
        C.Dispose();
    }
}

After doing that you can do something like:

public bool GetSomeValueById(Guid innerId)
{
    using(MyCompositeDisposable d = new MyCompositeDisposable(_uri, _id, innerId))
    {
        return d.C.GetSomeValue();
    }
}

Note that the MyCompositeDisposable will likely need to have try/finally blocks in the constructor and Dispose methods so that errors in creation/destruction properly ensure nothing ends up un-disposed.

The idea of wrapping it all in a class like this is perfect for my needs and offers just the right balance of code de-duplication and flexibility for all of my cases, plus it even helps in separation of concerns a bit. This had pretty much the best of all the answers. Thanks. — ckittel, Apr 25 '12 at 11:34
This has the same flaw as BFree's answer - an exception during the construction of C will leave A and B not disposed. — Amy B, Apr 25 '12 at 12:41
@DavidB I already had a note at the end of the answer that such error checking is needed, but that it wasn't included in the answer here. If it's needed in the OP's case he knows he needs to add it. — Servy, Apr 25 '12 at 13:43

Amy B · Answer 3 · 2012-04-23T18:29:49.957

You can always make a larger context in which to manage which objects should be created/disposed. Then write a method to create that larger context...

public class DisposeChain<T> : IDisposable where T : IDisposable
{
    public T Item { get; private set; }
    private IDisposable _innerChain;

    public DisposeChain(T theItem)
    {
        this.Item = theItem;
        _innerChain = null;
    }

    public DisposeChain(T theItem, IDisposable inner)
    {
        this.Item = theItem;
        _innerChain = inner;
    }

    public DisposeChain<U> Next<U>(Func<T, U> getNext) where U : IDisposable
    {
        try
        {
            U nextItem = getNext(this.Item);
            DisposeChain<U> result = new DisposeChain<U>(nextItem, this);
            return result;
        }
        catch  //an exception occurred - abort construction and dispose everything!
        {
            this.Dispose()
            throw;
        }
    }

    public void Dispose()
    {
        Item.Dispose();
        if (_innerChain != null)
        {
            _innerChain.Dispose();
        }
    }
}

Then use it:

    public DisposeChain<DataContext> GetCDisposeChain()
    {
        var a = new DisposeChain<XmlWriter>(XmlWriter.Create((Stream)null));
        var b = a.Next(aItem => new SqlConnection());
        var c = b.Next(bItem => new DataContext(""));

        return c;
    }

    public void Test()
    {
        using (var cDisposer = GetCDisposeChain())
        {
            var c = cDisposer.Item;
            //do stuff with c;
        }
    }

jrummell · Answer 4 · 2012-04-23T17:51:02.450

If your Dispoable types correctly dispose all disposable members, you would only need one using statement.

For example, this:

public bool GetSomeValueById(Guid innerId)
{
    using (var a = SomeFactory.GetA(_uri))
    using (var b = a.GetB(_id))
    using (var c = b.GetC(innerId))
    {
        return c.GetSomeValue();
    }
}

could become this if a had members of type's b and c, and a disposed of b and c in its dispose method:

public bool GetSomeValueById(Guid innerId)
{
    using (var a = SomeFactory.GetA(_uri))
    {
        return a.GetSomeValue();
    }
}

class A : IDisposable
{
  private a;
  private b;

  public A (B b, C c)
  {
     this.b = b; this.c = c;
  }

  public void Dispose()
  {
     Dispose(true);
  }

  protected void Dispose(bool disposing)
  {
     if (disposing)
     {
        b.Dispose();
        c.Dispose();
     }
  }
}

You would have to modify your factory to inject b and c into a, however.

You should be careful when having objects dispose of objects given to them by another class. What if more than one instance is relying on that object? Disposal should generally be the responsibility of the owning class, and in this case `A` does not own `b` and `c`. — Thomas, Apr 23 '12 at 17:53
@Thomas excellent point. Typically, you would also have boolean ctor parameters indicating whether or not A owns b and c. — jrummell, Apr 23 '12 at 17:56

How could one refactor code involved in nested usings?

4 Answers4