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?

• dfhwze: Rewriting C# again, eh? :-) Could you give an example where you would need this code fragment? foreach (var property in typeof(T).GetProperties()) /* .. */ disposableProperty.Dispose();
• t3chb0t: @dfhwze haha, as always :-P see the second quoted code, this is my current ugly use-case that this extension should replace. The refactored version is at the bottom.
• t3chb0t: @dfhwze maybe... but my imagination is currently unable to project it :-[
• HelloWorld: In c# 8.0 there is actually no need nestling for using statements ;) See: docs.microsoft.com/en-us/dotnet/csharp/whats-new/csharp-8
• t3chb0t: @HelloWorld this is nice! I'll definitely use it... someday, when it's official ;-)

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.

• t3chb0t: haha, I like the camel analogy.
• dfhwze: I stole this terminology in my answer, I hope you don't have a patent pending...
• Pieter Witvoet: @dfhwze: if I had, and it came to court, then we'd have a camel-case... ;)
• t3chb0t: ok, you both have good points for not using this invention ;-)

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.