Abuse of Closures? Violations of various principles? Or ok?

Question

Edit: fixed several syntax and consistency issues to make the code a little more apparent and close to what I actually am doing.

I've got some code that looks like this:

SomeClass someClass;
var finalResult = 
  DoSomething(() => 
  {
    var result = SomeThingHappensHere();
    someClass = result.Data;
    return result;
  })
  .DoSomething(() => return SomeOtherThingHappensHere(someClass))
  .DoSomething(() => return AndYetAnotherThing())
  .DoSomething(() => return AndOneMoreThing(someClass))
  .Result;

HandleTheFinalResultHere(finalResult);

where the DoSomething method is an extension method, and it expects a Func passed into it. So, each of the method calls in each of the DoSomething => lambda's returns a Result type.

this is similar to a Maybe monad. Except instead of checking for nulls, I am checking the status of the Result class, and either calling the Func that was passed into DoSomething or returning the previous Result without calling the Func

the problem i face is that want to have this kind of composition in my code, but i also need to be able to pass data from one of the composed call results into the call of another, as you can see with the someClass variable.

My question isn't whether or not this is technically correct... i know this works, because I'm currently doing it. My question is whether or not this is an abuse of closures, or command-query separation, or any other principles... and then to ask what better patterns there are for handling this situation, because I'm fairly sure that I'm stuck in a "shiny new hammer" mode with this type of code, right now.

Answer 1

Looks to me like you've built something very similar to a monad here.

You could make it a proper monad by making your delegate type a Func<SomeClass, SomeClass>, have some way to set up the initial SomeClass value to pass in, and have DoSomething pass the return value of one as the parameter of the next -- this would to make the chaining explicit rather than relying on lexically scoped shared state.

Answer 2

The weakness of this code is the implicit coupling between the first and second lambdas. I'm not sure of the best way to fix it.

Answer 3

As has already been noted, you've almost implemented a Monad here.

Your code is a bit inelegant in that the lambdas have side-effects. Monads solve this more elegantly.

So, why not turn your code into a proper Monad?

Bonus: you can use LINQ syntax!

---

I present:

LINQ to Results

 
Example:

var result =
    from a in SomeThingHappensHere()
    let someData = a.Data
    from b in SomeOtherThingHappensHere(someData)
    from c in AndYetAnotherThing()
    from d in AndOneMoreThing(someData)
    select d;

HandleTheFinalResultHere(result.Value);

With LINQ to Results, this first executes SomeThingHappensHere. If that succeeds, it gets the value of the Data property of the result and executes SomeOtherThingHappensHere. If that succeeds, it executes AndYetAnotherThing, and so on.

As you can see, you can easily chain operations and refer to results of previous operations. Each operation will be executed one after another, and execution will stop when an error is encountered.

The from x in bit each line is a bit noisy, but IMO nothing of comparable complexity will get more readable than this!

---

How do we make this work?

Monads in C# consist of three parts:

• a type Something-of-T,

• Select/SelectMany extension methods for it, and

• a method to convert a T into a Something-of-T.

All you need to do is create something that looks like a Monad, feels like a Monad and smells like a Monad, and everything will work automagically.

---

The types and methods for LINQ to Results are as follows.

Result<T> type:

A straightforward class that represents a result. A result is either a value of type T, or an error. A result can be constructed from a T or from an Exception.

class Result<T>
{
    private readonly Exception error;
    private readonly T value;

    public Result(Exception error)
    {
        if (error == null) throw new ArgumentNullException("error");
        this.error = error;
    }

    public Result(T value) { this.value = value; }

    public Exception Error
    {
        get { return this.error; }
    }

    public bool IsError
    {
        get { return this.error != null; }
    }

    public T Value
    {
        get
        {
            if (this.error != null) throw this.error;
            return this.value;
        }
    }
}

Extension methods:

Implementations for the Select and SelectMany methods. The method signatures are given in the C# spec, so all you have to worry about is their implementations. These come quite naturally if you try to combine all method arguments in a meaningful way.

static class ResultExtensions
{
    public static Result<TResult> Select<TSource, TResult>(this Result<TSource> source, Func<TSource, TResult> selector)
    {
        if (source.IsError) return new Result<TResult>(source.Error);
        return new Result<TResult>(selector(source.Value));
    }

    public static Result<TResult> SelectMany<TSource, TResult>(this Result<TSource> source, Func<TSource, Result<TResult>> selector)
    {
        if (source.IsError) return new Result<TResult>(source.Error);
        return selector(source.Value);
    }

    public static Result<TResult> SelectMany<TSource, TIntermediate, TResult>(this Result<TSource> source, Func<TSource, Result<TIntermediate>> intermediateSelector, Func<TSource, TIntermediate, TResult> resultSelector)
    {
        if (source.IsError) return new Result<TResult>(source.Error);
        var intermediate = intermediateSelector(source.Value);
        if (intermediate.IsError) return new Result<TResult>(intermediate.Error);
        return new Result<TResult>(resultSelector(source.Value, intermediate.Value));
    }
}

You can freely modify the Result<T> class and the extension methods, for example, to implement more complex rules. Only the signatures of the extension methods must be exactly as stated.