6

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
    }
}
  • 11
    I consider "not executing successfully" to be a failure to fulfill one's contract, so it would justify throwing an exception rather than returning an error code. That would make the code of the calling rmethod trivial. – Kilian Foth Nov 16 '17 at 15:34
  • Is this "I have a class where N methods all look very similar" or "I have one method that looks like an arrow"? – Caleth Nov 16 '17 at 16:03
  • 1
    Possible duplicate of Approaches to checking multiple conditions? – gnat Nov 16 '17 at 16:07
  • @gnat How do you do it? :) How do you almost always come up with a possible duplicate? Just plain search? Or some hidden SE feature that I'm not aware of? – Zapadlo Nov 16 '17 at 16:15
  • 4
    @Zapadlo fun fact: for a long time I thought gnat was a duplicate-searching bot. – BgrWorker Nov 17 '17 at 16:37

10 Answers 10

4

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/

  • 2
    How does your mechanism incorporate deinitialisation/finalisation code, such as the ReleaseSingleUserMode() method in the original post? – Ant_222 Dec 30 '17 at 20:22
  • @Ant_222 ReleaseSingleUserMode() would just be the last call in the chain. If it needs to operate differently based on whether or not the previous calls all succeeded, then it should be outside the chain. – Graham Jan 2 '18 at 15:14
  • ReleaseSingleUserMode() should be called if and only if SingleUserMode() succeeded. I see no way to do it using your proposal. Perhaps it would be clearer if you applied it to the OP's actual question rather than to an unrelated problem with cars and users... – Ant_222 Jan 13 '18 at 14:56
  • Sure thing, see edits above. I've tried a few diff approaches to deal with the problem you mention, that a call later in the chain needs to fire even if there's some other failures, and the simplest answer is that in that case, its not really part of the core chain and should be moved out. Also, if practically every other method in the chain needs that kind of conditional logic, then I don't think there's any short cut to a lot of if/else's. – Graham Jan 17 '18 at 14:19
3

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
    }
}
  • changing methods to throw exceptions rather than return bools obvs – Ewan Jan 2 '18 at 20:22
  • You didnt miss anything, I was a bit against throwing excpetions and thought about it being bad design, for this. Since, I tend to not use excpetion to have a control flow. – Rand Random Jan 8 '18 at 11:02
  • 1
    @RandRandom I agree about not using them for control flow. But in this case isn't it always an error if one of your if statements is hit? If not, I would add a if(singleUserMode.IsRequired) before attempting to 'open' it – Ewan Jan 8 '18 at 11:07
1

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.

1

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.

  • Noticed the lost excpetion, while playing around, what "api" fits me best. Was kind of surprised that this happened, but its understandable. – Rand Random Jan 8 '18 at 11:00
1

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
}
  • Your suggestion of using and IDisposable is what I talked about in my answer. It should be pointed out that if ReleaseSingleUserMode throws an exception, then the exception will be thrown out of Dispose(), which should not happen. And if you were to use try/finally, then exception in finally will cause the first exception to be lost. – Euphoric Jan 2 '18 at 15:27
  • I meant to say that SingleUserMode is optional, the rest of the code could be or could not be wraped with a SingleUserMode. – Rand Random Jan 8 '18 at 10:58
1

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();
    }
}
1

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.

-1

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.

  • This will not release SingleUserMode. – JacquesB Jan 2 '18 at 12:12
-2

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.

  • That might work if the individual functions actually threw exceptions, but the OP's code suggests that these functions return bool success instead. – Robert Harvey Nov 16 '17 at 17:27
  • I would change that to have them throw exceptions, maybe I should have put that in my answer. Functions that return bools end up making code with lots of IFs in the calling function. If the function returns "false" a lot (not the exception, but the rule), then throwing an exception may not be advised as caching all those exceptions may result in a performance hit. – Jon Raynor Nov 16 '17 at 17:45
  • I have grown to really dislike using Exceptions for control flow, because, as you say above, you end up throwing/catching a LOT of exceptions for things like validation. – Graham Nov 16 '17 at 18:30
  • @Graham - I am not advocating using exceptions for control flow, I am advocating using them to catch fails. Sure, I agree, if you are validating, that a case where exceptions should be not used and a true/false can be returned. – Jon Raynor Nov 16 '17 at 19:13
  • 1
    If there is an exception in SaveChanges, this solution will not release single user mode. – JacquesB Jan 2 '18 at 11:35
-3

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 );
        }
    }
  • To whoever has down-voted my answer, may I make so bold as to ask what made you do so if not religious fear of the goto statement? Do you prefer exceptions, and using's, which not only disrupt the flow of control as goto's do, but also introduce unwanted nesting into conceptually sequential (flat) code? – Ant_222 Jan 1 '18 at 21:41
  • I guess the OP had this coming by using result codes rather than exceptions... – JacquesB Jan 2 '18 at 11:40
  • The code is not "flat". For example SingleUserMode and ReleaseSingleUserMode are clearly logically coupled - you just hide that with the different error labels with the names Error and ErrorSm. This is exactly why goto's are discouraged - the hide the logical structure of the code. – JacquesB Jan 2 '18 at 12:09
  • 1
    your code is a messier version of using exceptions – Austin_Anderson Jan 2 '18 at 13:59
  • 1
    @Ant_222: Sure, and function calls are like a stack, so all function calls can be transformed into goto's. That does not mean it is a good idea. – JacquesB Jan 2 '18 at 14:03

Not the answer you're looking for? Browse other questions tagged or ask your own question.