Calling multiple "fail-able" code parts in a row

Question

I have the following methods:

• SingleUserMode() - turns the single user mode on in the database
• BeginEdit() - starts the edit of items in the context, locks them in the database
• SaveChanges() - saves the changes to the database
• ReleaseSingleUserMode() - turns the single user mode off in the database

Each method returns an object which has a message and if it is was executed successfully.

I would like to know if there is a "nicer" way than to write this:

using (var storeContext = StoreContext.CreateContext())
{
    var result = storeContext.SingleUserMode();
    if (result)
    {
        //load data into storecontext...

        result = storeContext.BeginEdit();
        if (result)
        {
            //change data...

            result = storeContext.SaveChanges();
            if (result)
            {
                //update UI on success
            }
        }

        var releaseResult = storeContext.ReleaseSingleUserMode();
        if (!releaseResult)
        {
            result.AddSubResult(releaseResult); //add the failed release of single user mode to the "parent" result
        }
    }

    if (!result)
    {
        //fire messagebox on error
    }
}

(The comments indicate the places where usually there is a custom code, the rest is "always" the same)

FYI, the methods SingleUserMode and ReleaseSingleUserMode are optional and dont have to be called, though if you call SingleUserMode you have to call ReleaseSingleUserMode, but without it its just this:

using (var storeContext = StoreContext.CreateContext())
{
    //load data into storecontext...

    result = storeContext.BeginEdit();
    if (result)
    {
        //change data...

        result = storeContext.SaveChanges();
        if (result)
        {
            //update UI on success
        }
    }

    if (!result)
    {
        //fire messagebox on error
    }
}

Answer 1

I recently went through this exact scenario! I started by creating a Result class with a few helper methods:

public class Result
{
    public bool Success { get; set; }
    public ValidationResult[] TransactionErrors { get; private set; }

    // Combines param Result errors into this Result instance
    public Result MergeWithErrorsFrom(Result transaction)
    {
        if (transaction == null) return this;
        TransactionErrors = TransactionErrors.Concat(transaction.TransactionErrors).ToArray();
        if (TransactionErrors.Any() || !transaction.Success)
            Success = false;
        return this;
    }

    // Method will execute each function in the params Func list until one fails, combining the error messages in the current instance. Returns 'this'.
    public Result CombineWithUntilFailure(params Func<Result>[] executionFuncList)
    {
        foreach (var func in executionFuncList)
        {
            this.MergeWithErrorsFrom(func()); // execute the Func and copy errors to current instance.
            if (this.TransactionErrors.Any() || !this.Success) // stop exec'ing if we get any kind of error.
                break;
        }
        return this;
    }

    public void AddTransactionError(string errorMessage, string[] memberNames = null)
    {
        var newError = new ValidationResult(errorMessage, memberNames ?? new[] { "" });
        TransactionErrors = TransactionErrors.Concat(new[] { newError }).ToArray();
        Success = false;
    }

    public static Result FailIf(Func<bool> func, string errorMessage)
    {
        var callResult = new Result();
        if (func())
            callResult.AddTransactionError(errorMessage);
        return callResult;
    }

    public static Result Empty() => new Result { Success = true };

    public static Result Empty(Action action)
    {
        action();
        return new Result { Success = true };
    }

    public static Result CombineUntilFailure(params Func<Result>[] executionFuncList) =>
        new Result().CombineWithUntilFailure(executionFuncList);
}

public class Result<T> : Result
{
    public T Payload { get; set; }
}

Next, I made sure all my validation and transactional methods returned a Result or Result<T>. That way, I could chain them together using the CombineWithUntilFailure function from above. The resulting code would look like this:

using (var storeContext = StoreContext.CreateContext())
{
    var releaseNeeded = false;
    var result = Result.CombineUntilFailure(
        () => storeContext.SingleUserMode(),
        () => Result.Empty(() => releaseNeeded = true),
        () => storeContext.BeginEdit(),
        () => storeContext.SaveChanges());

    if (releaseNeeded)
        result.CombineWithUntilFailure(() => storeContext.ReleaseSingleUserMode());

    if (!result.Success) { /*show errors here*/  }

    return result;
}

If any of the Func params above return an invalid Result, the chain short-circuits and the following ones are not executed. My actual code for the Result class includes things like extra success messaging for logging and such, which can be built up in the same way as the error messages are, if you need that functionality.

I learned this technique from an article that described it as "Railway Oriented Programming" found here: https://fsharpforfunandprofit.com/rop/

Answer 2

Am I missing something? you don't need anything clever here

using (var storeContext = StoreContext.CreateContext())
{
    try
    {
        storeContext.SingleUserMode();
        storeContext.BeginEdit();
        storeContext.SaveChanges();
        //update UI on success
    }
    catch(SingleUserModeException ex)
    {
        //fire messagebox on error
        throw; //prevent further execution
    }
    catch(Exception ex)
    {
        //fire messagebox on error
        //alternatively append errors to a list for later display
    }

    //continue because the single user mode did not fail
    try
    {
        storeContext.ReleaseSingleUserMode();
    }
    catch
    {
        //fire messagebox on error
    }
}

Answer 3

I'm in favor of composable objects, thus Decorator pattern. I come up with small, cohesive and resuable objects. It looks more OOP-like, and I won't forget, for example, to close started transaction and alike. So here is how I'd make it (sorry for php):

interface Response
{
    public function display();
}

interface Action
{
    public function go(): Response;
}

class SingleModeOn
{
    private $a;
    private $c;

    public function __construct(Action $a, Context $context)
    {
        $this->a = $a;
        $this->c = $context;
    }

    public function go(): Response
    {
        if ($this->c->singleUserMode()) {
            $r = $this->a->go();
            $this->c->releaseSingleUserMode();
            return $r;
        } else {
            return new SingleModeFailed();
        }
    }
}

class Transactional implements Action
{
    private $a;
    private $c;

    public function __construct(Action $a, Context $context)
    {
        $this->a = $a;
        $this->c = $context;
    }

    public function go(): Response
    {
        if ($this->c->beginEdit()) {
            $response = $this->a->go();
            $this->c->finishEdit();
            return $response;
        } else {
            return new TransactionFailed();
        }
    }
}

(new SingleModeOn(
    new Transactional(
        new YourItemSavingScenarioName(
            new UINotification()
        )
    )
))
    ->go()
        ->display()
;

No nested ifs, no exceptions for flow control.

Answer 4

I would definitely recommend exceptions. After some quick hacking, I came up with this.

class Program
{
    static void Main(string[] args)
    {
        using (var storeContext = StoreContext.CreateContext())
        {
            try
            {
                var releasable = storeContext.SingleUserModeAsReleasable();
                releasable.ReleaseAfter(() =>
                {
                    //load data into storecontext...

                    storeContext.BeginEdit(); // will throw exception when error

                    //change data...

                    storeContext.SaveChanges(); // will throw exception when error

                    //update UI on success
                });
            }
            catch (Exception)
            {
                //fire messagebox on error
            }
        }
    }
}

public interface IReleasable
{
    void Release();
}

public static class ReleasableExtension
{
    public static void ReleaseAfter(this IReleasable releasable, Action action)
    {
        Exception[] exceptions = new Exception[2];
        try
        {
            action();
        }
        catch (Exception ex)
        {
            exceptions[0] = ex;
        }

        try
        {
            releasable.Release();
        }
        catch (Exception ex)
        {
            exceptions[1] = ex;
        }

        var thrownExceptions = exceptions.Where(x => x != null).ToArray();
        if(thrownExceptions.Length != 0)
        {
            throw new AggregateException(thrownExceptions);
        }
    }
}

public static class StoreContextSingleUserMode
{
    private class SingleUserModeRelease : IReleasable
    {
        private StoreContext storeContext;

        public SingleUserModeRelease(StoreContext storeContext)
        {
            this.storeContext = storeContext;
        }

        public void Release()
        {
            storeContext.ReleaseSingleUserMode(); // will throw exception when error
        }
    }

    public static IReleasable SingleUserModeAsReleasable(this StoreContext storeContext)
    {
        storeContext.SingleUserMode(); // will throw exception if error, won't even bother releasing

        return new SingleUserModeRelease(storeContext);
    }
}

The major problem is need to release the SingleUserMode if it was successfully acquired. Which is why I encapsulated it together into a IReleasable object. This then allows to create continuation-like code that properly handles the exceptional state. One cosmetic problem is that the code is inside-out. So it is not obvious how it will flow.

This is basically clone of IDisposable interface. But IDisposable cannot be used, as you should not throw an exception from Dispose. And even then, if there is exception in finally block, then previous exception is lost. Which is not behavior your want. So I made a clone with proper behavior, that throws aggregate exception if both inner code and Release code throws an exception.

Answer 5

What does "nicer" mean? If you mean "easier to read and maintain", then you should use more idiomatic C# code. The idiomatic way of signaling errors in C# is through exceptions. Your use of a result value flag to signal errors is not idiomatic, which means it is not nice to the reader.

If you change the code to use exceptions, it could look like this:

using (var storeContext = StoreContext.CreateContext())
{
    try 
    {
        //load data into storecontext...
        storeContext.BeginEdit();
        //change data...
        storeContext.SaveChanges();
        //update UI on success
    }
    catch (Exception ex)
    {
      //fire messagebox on error
    }
} 

I have left out SingleUserMode and ReleaseSingleUserMode since you state they are not needed. You shouldn't have code which doesn't have a purpose. But if it turns out you need this method-pair, I suggest you encapsulate them in IDsposable class, since then you can ensure single user mode is always released, e.g. like:

using (new SingleUserMode()) 
{
  // do something in single user mode
}

Answer 6

Asssuming you need to implement this multiple times, you could create something like a transaction class, like below:

public void Execute() {
    using (var storeContext = StoreContext.CreateContext())
    {
        var caller = new Caller<StoreContext>(storeContext);

        caller.RegisterSetup(s => s.SingleUserMode(), s => s.ReleaseSingleUserMode());
        caller.OnSuccess(s => UpdateUi());
        caller.OnError(s => ShowMessageBoxWithErrorMessages());

        //Your transaction
        caller.Call(s => {
            //load data into storecontext...
            s.BeginEdit();
        });

        caller.Call(s => {
            //change data...
            s.SaveChanges();
        });

        //This will call all Setup functions; and keep calling all functions from "Call" calls; for errors it will call "OnError", otherwise "OnSuccess" at the end, and finally the "Teardown" functions registered along with the "Setup" functions.
        caller.Commit();
    }
}

Answer 7

Your limiting factor is that you have a return result that you need to key off of to handle UI updates. This removes the ability to create a new IDisposable to handle your SingleUserMode state. Let's say you have a new object to represent the mode:

public class SingleUserMode : IDisposable
{
    final StoreContext context;

    public SingleUserMode(StoreContext contextIn)
    {
        context = contextIn;
        IsValid = context.SingleUserMode();
    }

    public bool IsValid { get; }

    public void Dispose()
    {
        context.ReleaseSingleUserMode();
    }
}

This allows your StoreContext to have a new method to use this disposable object:

public SingleUserMode EnterSingleUserMode()
{
    return new SingleUserMode(this);
}

Which in turn allows your code to be simplified a bit:

using (var storeContext = StoreContext.CreateContext())
using (var singleUser = storeContext.EnterSingleUserMode())
{
    if (singleUser.IsValid)
    {
        // do good stuff
    }
    else
    {
        //fire messagebox on error
    }
}

In order to fully take advantage of this pattern, you will have to refactor some things, but hopefully not too bad. The using construct works with every IDisposable, and Dispose() is guaranteed to be called once you leave the scope of the using statement. It is a very good C# idiom to use for operations that require cleanup.

What complicates things is the way your current logic works for good/bad state.

Answer 8

This sort of structure suggests using the short circuit evaluation of Boolean expressions. Execution stops when a method returns false. Also move the extra processing on the conditionals to helper functions.

    result=    storeContext.SingleUserMode()
            && handle_SingleUserMode()     // this should always return true
            && storeContext.BeginEdit()
            && handle_BeginEdit()          // this should always return true
            && storeContext.SaveChanges()
            && handle_SaveChanges();       // this should always return true

I think this is easy to read code.

Answer 9

This should be better than all the ifs...

try
{
    SingleUserMode();
    BeginEdit();
    SaveChanges();
}
catch (Exception ex)
{
    //Log
}
finally
{
    //Cleanup
    ReleaseSingleUserMode();
}

Or maybe this...

try
{
    using (var singleUserMode = SingleUserMode())
    {
        BeginEdit();
        SaveChanges();
    } //Release happens on Dispose...
}
catch (Exception ex)
{
    //Log
}

Although, I think this is a minor abuse of IDisposable.

Answer 10

Since your error-handing logic is linear, any nested structures, be they try blocks or nested if statements, are unfit to express it. I therefore suggest that you employ the goto statement as shown below. I have expressed the operations of loading data, changing it, and updating the UI, as methods that return a success flag and assign an error message to an out string parameter:

    storeContext = StoreContext.CreateContext();
    result = storeContext.SingleUserMode();
    if( !result ) goto Error;

    if( !LoadData( storeContext, out errMsg ) )
    {   result = NewResult( errMsg );
        goto ErrorSm;
    }

    result = storeContext.BeginEdit();
    if( !result ) goto ErrorSm;

    if( !ChangeData( storeContext, out errMsg ) )
    {   result = NewResult( errMsg );
        goto ErrorSm;
    }

    result = storeContext.SaveChanges();
    if( !result ) goto ErrorSm;

    // why can't you update UI in the very end?
    if( !UpdateUiOnSuccess( out errMsg ) )
    {   result = NewResult( errMsg );
        goto ErrorSm;
    }

ErrorSm:
    releaseResult = storeContext.ReleaseSingleUserMode();
    if( !releaseResult ) result.AddSubResult( releaseResult );

Error: storeContext.Dispose()

if( !result )
{   /* show error */ }
else
{   /* I suggest to update UI here! */ }

But I still do not understand what your result object is. You are using it as a boolean, yet it somewhy has an AddSubResult() method. Such readability problems are one of my objections to the var keyword.

Edit: Here is a version without goto's, with reinstated using(), and a boolean flag that determines whether to enter single-user mode (which you said was optional):

    using( storeContext = StoreContext.CreateContext() )
    {   while( true )
        {   if( singleUser )
            {   result = storeContext.SingleUserMode();
                if( !result ) break;
                singleUserEntered = true;
            }

            if( !LoadData( storeContext, out errMsg ) )
            {   result = NewResult( errMsg );
                break;
            }

            result = storeContext.BeginEdit();
            if( !result ) break;

            if( !ChangeData( storeContext, out errMsg ) )
            {   result = NewResult( errMsg );
                break;
            }

            result = storeContext.SaveChanges();
            if( !result ) break;

            if( !UpdateUiOnSuccess( out errMsg ) )
            {   result = NewResult( errMsg );  }

            break;
        }

        if( singleUserEntered )
        {   releaseResult = storeContext.ReleaseSingleUserMode();
            if( !releaseResult ) result.AddSubResult( releaseResult );
        }
    }