Reducing using/foreach/using nesting with a helper extension

Question

Problem statement

There are sometimes foreach scenarios that require deep nesting due to multiple disposable objects involved that look like this:

using(..)
{
    foreach(..)
    {
        using(..)
        {

        }
    }
}

or a real-world example from one of my applications:

using (_logger.BeginScope().CorrelationHandle("TestBundle").AttachElapsed())
using (Disposable.Create(() =>
{
    foreach (var item in cache.Values) item.Dispose();
}))
{
    _logger.Log(Abstraction.Layer.Service().Meta(new { TestBundleFileName = testBundle.FileName }));
    foreach (var current in tests)
    {
        using (_logger.BeginScope().CorrelationHandle("TestCase").AttachElapsed())
        {
            // body
        }
    }
}

I find this code is very ugly and there is virtually no other way of writing this (or it's just me and I cannot think of any).

---

Suggested solution

So, in order to make it prettier I created the Using extension that encapsulates the outer and inner usings within the loop.

Its body is identical to the using/foreach/using pattern I'd like to get rid of elsewhere:

public static class EnumerableExtensions
{   
    public static IEnumerable<(TItem Current, TInner Context)> Using<TItem, TOuter, TInner>
    (
        this IEnumerable<TItem> source,
        TOuter outer,
        Func<TItem, TInner> inner
    )
    {
        using (new Disposer<TOuter>(outer))
        {
            foreach (var item in source)
            {
                using (var context = new Disposer<TInner>(inner(item)))
                {
                    yield return (item, context);
                }
            }
        }
    }   
}

This is using a helper struct for disposing TOuter and TInner

public readonly struct Disposer<T> : IDisposable
{
    public Disposer(T value)
    {
        Value = value;
    }

    public T Value { get; }

    public void Dispose()       
    {
        if (Value is IDisposable disposable)
        {
            disposable.Dispose();
        }
        else
        {
            foreach (var property in typeof(T).GetProperties())
            {
                if (property.GetValue(Value) is IDisposable disposableProperty)
                {
                    disposableProperty.Dispose();
                }
            }
        }
    }

    public static implicit operator T(Disposer<T> disposer) => disposer.Value;
}

And here is the Disposable helper:

public class Disposable : IDisposable
{
    private readonly Action _dispose;

    private Disposable(Action dispose)
    {
        _dispose = dispose;
    }

    public static IDisposable Empty => new Disposable(() => { });

    public static IDisposable Create(Action dispose)
    {
        return new Disposable(dispose);
    }

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

Example usage

Now, when I have such a scenario, I can put everything inside the loop:

void Main()
{
    var numbers = new[] { 1, 2, 3 };

    foreach (var (item, context) in numbers.Using
    (
        outer: Disposable.Create (() => Console.WriteLine("Outer disposed.")), 
        inner: _ => Disposable.Create (() => Console.WriteLine("Inner disposed.")))
    )
    {
        item.Dump();
        context.Dump();
    }
}

Then, when I refactor the previous ugly piece of code with this extension it turns into this:

foreach (var (current, context) in tests.Using
(
    outer: new 
    {
        Scope = _logger.BeginScope().CorrelationHandle("TestBundle").AttachElapsed(),
        CleanUp = Disposable.Create(() =>
        {
            foreach (var item in cache.Values) item.Dispose();
        })
    }, 
    inner: _ => _logger.BeginScope().CorrelationHandle("TestCase").AttachElapsed())
)
{
    // body
}

Questions

• What do you think of this helper?
• Would you say the code is now easier to read?
• Can the Using extension or the Disposer<T> be improved?

Answer 1

I'd argue that no, this does not make the code easier to understand. Instead of a combination of familiar general-purpose constructs (using and foreach), you now have an undocumented custom special-purpose construct Using(outer, inner) that must be understood before one can make sense of the code. This use-case seems too uncommon to be worth that trade-off.

The main problem I have with the original piece of code is that it looks like a 'camel' because of the Disposable.Create hump at the start. Your alternative is still a camel, just with a bigger hump at the front and a flatter one at the back. With a DisposeAll extension method, you could turn it into a dromedary. It's still a tall hump, but now the shape actually matches its single-loop nature:

using (_logger.BeginScope().CorrelationHandle("TestBundle").AttachElapsed())
using (Disposable.Create(() => cache.Values.DisposeAll()))
{
    _logger.Log(Abstraction.Layer.Service().Meta(new { TestBundleFileName = testBundle.FileName }));
    foreach (var current in tests)
    {
        using (_logger.BeginScope().CorrelationHandle("TestCase").AttachElapsed())
        {
            // TODO
        }
    }
}

Another thing you can do is to move the loop body to a separate method or local function.

Answer 2

I agree with Pieter Witvoet it doesn't improve readability. What you could do, since I know you like lambda's, is make additional convenience helpers to reduce camel code.

Disposable.Create(() =>
{
    foreach (var item in cache.Values) item.Dispose();
})

Disposable.Create(cache.Values, (item) => item.Dispose());

Additional method:

public static IDisposable Create<T>(IEnumerable<T> source, Action<T> action)
{
    return new Disposable(() =>
    {
        foreach (var item in source) action(item);
    });
}

I am not a big fan of this:

foreach (var property in typeof(T).GetProperties())
{
    if (property.GetValue(Value) is IDisposable disposableProperty)
    {
        disposableProperty.Dispose();
    }
}

Can we really assume all of the properties that implement IDisposable want to be disposed here? In addition, property.GetValue(Value) throws errors in this naive GetProperties call.