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.

Condescending

It's not very easy to read.... – Beacon

100% agreed. sadly, it's still a significant improvement over what i started with. a little more formatting might help... might... – Condescending

So essentially you're trying to make a series of independent method calls and combine the results of each call into a single result. Is that correct? Or what exactly are you trying to achieve? – Heliotropism

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

You don't need the return keyword for single line lambdas. Just sayin'. – Retinoscope

If it was called ComposeWith instead of DoSomething, it might not be so bad. – Suzisuzie

It would be more idiomatic C# to throw a custom exception carrying the information you need in the case of a bad Result. – Prom

@mquander - exceptions are for exceptional situations... not flow control and logic. they are horribly expensive and create awful, ugly code, and should be reserved for situations where the system can't recover. this is not that situation. this is just returning the result of a process so the system can decide what to do next. – Condescending

@Derick: I mostly agree, and I'd personally be more inclined to write what you wrote. I think a lot of C# developers would use an exception for this kind of flow control, though. – Prom

Answer 1

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.

Heliotropism

Awesome. Been puzzling monads for a while and this made a lot of sense and definately helped my understanding. Could you not move much of the logic in Select / SelectMany extensions into a Bind<TResult>( Func<TSource,TResult> ) or similar? Not entirely sure on the syntax but it would remove a lot of the duplication in the extension methods. – Buchenwald

@Neal: You can express the first two extension methods in terms of the third extension method. – Heliotropism

Yes, the reason I asked the question is that the rules are implemented as extension methods meaning that you have moved the behaviour of the Result<T> class outside the scope of the class. If that behaviour was moved into the class then it becomes useful by itself and the extension methods simply become a bridge between Result<T> and linq. – Buchenwald

Answer 2

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.

Valenzuela

yes, i've been studying / learning monads recently and i was trying to create something along those lines. problem with your suggestion is that i need to always return the "result" class from the function, and in one case i need to also get the "someClass" data out of one function into the next. – Condescending

+1 Side effects are hard to reason about and test. You'll thank yourself later if you refactor to Func instead of Action and explicitly chain operations. – Allomorph

Why is leaving the result type so critical? Is it being passed around through someone else's code you can't modify before coming back to have another link in the composition chain tacked on? – Valenzuela

the DoSomething method has several checks and pieces of logic in it that use the result type to determine whether or not to call the Func that is passed in to get the next result, or simply return the current result. – Condescending

Waitaminute. If Result has a member of type SomeClass (I'm guessing it's some kind of criteria parameter block) why not use that member to pass it down the lambda chain rather than creating a separate mutable variable that's visible to all the lambdas? – Valenzuela

Answer 3

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.

Decoteau

definitely. it's bad design and semantic coupling - you have to know what is happening under the hood to understand the ramifications of pulling the value out of one and into the other. – Condescending