Indenting lambdas and nested actions

Question

When using lambdas, usually on TPL, I get lost on indentation... Are there some best practices to format this? For example, take this code:

Task t1 = factory.StartNew(() =>
{
    DoSomething();
}
.ContinueWith((t2) =>
{
    DoSomethingWhenComplete();
}, TaskContinuationOptions.OnlyOnRanToCompletion)
).ContinueWith((t3) =>
{
    DoSomethingOnError();
}, TaskContinuationOptions.OnlyOnFaulted);

1. Is my "indentation" correct?
2. In that example, I want to execute t1, then if it finished OK, execute t2, and on error execute t3. But looks as though t3 is a continuation from t2, not from t1... What do I need to fix in that code to get the correct behaviour? I think that I'm lost on that indentation or missing some parenthesis...

Answer 1

There are some best practice to format this?

I'm not aware of any. Your formatting looks OK by me (besides the notes below). Alternatively, you may just follow Visual Studio automatic formatting (try Format Document from Editor/Advanced menu).

In that example, I want execute t1 then if finish ok execute t2 and on error execute t3. But looks that t3 is continuation from t2, not from t1... What I need fix that code to correct behaviour? I think that I'm lost on that indentation or missing some parenthesis...

The code fragment from your question would not even compile. You probably wanted this:

Task t1 = factory.StartNew(() =>
{
    DoSomething();
});

t1.ContinueWith((t2) =>
{
    DoSomethingWhenComplete();
}, TaskContinuationOptions.OnlyOnRanToCompletion);

t1.ContinueWith((t2) =>
{
    DoSomethingOnError();
}, TaskContinuationOptions.OnlyOnFaulted);

This may work, but you're missing another state: OnlyOnCanceled. I'd rather handle all completion statuses of t1 in the same place:

Task t1 = factory.StartNew(() =>
{
    DoSomething();
}).ContinueWith((t2) =>
{
    if (t2.IsCanceled)
        DoSomethingWhenCancelled();
    else if (t2.IsFaulted)
        DoSomethingOnError(t1.Exception);
    else
        DoSomethingWhenComplete();
});

This still might be missing one thing: your code will be continued on a random pool thread without synchronization context. E.g, if you called ContinueWith on a UI thread, you won't be able to access the UI inside DoSomething* methods. If that's not what you expected, explicitly specify the task scheduler for continuation:

Task t1 = factory.StartNew(() =>
{
    DoSomething();
}).
ContinueWith((t2) =>
{
    if (t1.IsCanceled)
        DoSomethingWhenCancelled();
    else if (t1.IsFaulted)
        DoSomethingOnError(t1.Exception);
    else
        DoSomethingWhenComplete();
}, TaskScheduler.FromCurrentSynchronizationContext());

If you need to target .NET 4.0 but use VS2012+ as your development environment, consider using async/await and Microsoft.Bcl.Async instead of ContinueWith. It's much easier to code, more readable and will give you structured error handling with try/catch.

If you can't use async/await, consider using Task.Then pattern by Stephen Toub for task chaining (more details here).

Answer 2

1. I am not sure myself the best way to do indentation. If I were to write an equivalent to your code with minimal change in meaning, I might end up with something like the following. Sorry, it’s not a problem I’ve solved myself:

   Task t1 = Task.Factory.StartNew(
       () =>
       {
           DoSomething();
       })
       .ContinueWith(
           (t2) =>
           {
               DoSomethingWhenComplete();
           },
           TaskContinuationOptions.OnlyOnRanToCompletion)
       .ContinueWith(
           (t3) =>
           {
               DoSomethingOnError();
           },
           TaskContinuationOptions.OnlyOnFaulted);
   

   My reasoning behind the indentation I used is that a “child”/“part” of an expression should be indented deeper than the line where its “parent”/“container” begins. In the prior lines, the method’s arguments are part of a method call. So if the method call itself is at one level of indentation, the arguments should be indented one level further:

   MethodCall(
       arg1,
       arg2);
   

   Likewise, both sides of a binary operator such as scope resolution/member access (.) are children of the expression and we can, in a way, think of them all being at the same level. For example, there may be a.b.c or a + b + c, and I would consider each element to be a child of the overall expression. Thus, since each .ContinueWith() is part of the overall statement started on the first line, they should also each be indented just like in the following multiline arithmetic expression:

   var x = 1
       + 2
       + SomethingComplicated();
   

2. One big point of the Task Parallel Library is to enable you to continue writing “normal” looking code. What you have done is written a lot of code and calls to the TPL to reimplement a feature that already exists in C#—the try{}catch{}finally{} block. Also, using Task.Run(), assuming you just wanted to hoist the operation to the threadpool (but you can easily change this back to use your custom TaskFactory).

   Task t1 = Task.Run(
       () =>
       {
           try
           {
               DoSomething();
           }
           catch (Exception ex)
           {
               DoSomethingOnError(ex);
               // Error: do not proceed as usual, but do not mark t1
               // as faulted. We did something magical in
               // DoSomethingOnError() that addresses the error so
               // that it doesn’t need to be reported back to our
               // caller.
               return;
           }
   
           // Completed successfully!
           DoSomethingWhenComplete();
       });
   

   The way I handled the error in catch{} by calling DoSomethingOnError() and immediately returning simulates the way faultedTask.ContinueWith(action, TaskContinuationOptions.OnlyOnFaulted) behaves. The result of that expression is a Task representing the continuation. The continuation’s Task will only fault if the continuation itself faults. So by assigning the continuation to t1 rather than the original Task, you are effectively catching and swallowing the exception, just like how I catch and swallow it in my try{}catch{}. Just the lambda I wrote does this much more clearly than trying to compose a bunch of continuations manually.

   As @Noseratio says, you get much clearer code if you use async/await when it makes sense to do so. For offloading some standard, non-async method calls to the threadpool as your question suggests, it is not obvious that moving to async/await would actually help you. But replacing a bunch of TPL API calls with a single lambda does seem like a worthwhile refactor.