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
    }
}
Rand Random
  • 173
  • 4
  • 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?](https://softwareengineering.stackexchange.com/questions/191208/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? – Vadim Samokhin 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
  • @BgrWorker actually that's exact conclusion I came to yesterday! But then I checked his profile and saw his answers. So if he's a bot, then definitely a quite intelligent one. – Vadim Samokhin Nov 17 '17 at 16:45
  • This account is shared by bot and a person. – Basilevs Jan 02 '18 at 11:10
  • What is the type of result? Is it bool or is it something that has an operator true and an operator false? There are some interesting approaches that could be applied but it really depends on what the individual methods return whether or not those approaches make sense – Aluan Haddad Jan 02 '18 at 14:06
  • Are you sure that putting the database into single user mode really is the right thing to do, and that you can't achieve your goal using transactions and isolation? – Thomas Carlisle Jan 02 '18 at 16:23
  • One of the problems of throwing exceptions for this kind of validation is that it requires a lot of thought/effort put into the question of deciding what to show to the user. You will either have to have complex `catch` blocks checking for different sub-types of Exception that you create, or perhaps a single common sub-type of Exception with something like a 'UserMessage' field. I have worked on lots of apps where system details leaked into error screens because dev's were writing stuff like `throw new Exception("Please enter your name")`. – Graham Jan 02 '18 at 20:54
  • @Basilevs can you explain yourself? dont really know what you mean – Rand Random Jan 08 '18 at 10:49
  • @AluanHaddad in my case its a custom class which has cast overriden for boolean and therefor can be used as if (result), all the above methods return the same result object with a string message on fail – Rand Random Jan 08 '18 at 10:51
  • @ThomasCarlisle sadly this is a design decision made by my boss which we tried at some point to neglect but he made a "final decision", the main reason for him was to "absolutly" block all other users when doing something "big" – Rand Random Jan 08 '18 at 10:52
  • @Graham thx for your insight, will keep it in mind – Rand Random Jan 08 '18 at 10:54

10 Answers10

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/

Graham
  • 3,833
  • 1
  • 21
  • 18
  • 2
    How does your mechanism incorporate deinitialisation/finalisation code, such as the `ReleaseSingleUserMode()` method in the original post? – Anton Shepelev 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 02 '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... – Anton Shepelev 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
    }
}
Ewan
  • 70,664
  • 5
  • 76
  • 161
  • changing methods to throw exceptions rather than return bools obvs – Ewan Jan 02 '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 08 '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 08 '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.

Vadim Samokhin
  • 2,158
  • 1
  • 12
  • 17
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.

Euphoric
  • 36,735
  • 6
  • 78
  • 110
  • 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 08 '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
}
JacquesB
  • 57,310
  • 21
  • 127
  • 176
  • 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 02 '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 08 '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();
    }
}
Emerson Cardoso
  • 2,050
  • 7
  • 14
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.

Berin Loritsch
  • 45,784
  • 7
  • 87
  • 160
-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.

cwallach
  • 327
  • 1
  • 7
-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.

Rand Random
  • 173
  • 4
Jon Raynor
  • 10,905
  • 29
  • 47
  • 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 02 '18 at 11:35
  • I think that the invocation of `SingleUserMode()` should be outside the `try` block, because `ReleaseSingleUserMode()` should be called *only* upon a successful entry into single-user mode. – Anton Shepelev Jan 03 '18 at 10:34
-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 );
        }
    }
Anton Shepelev
  • 202
  • 2
  • 5
  • 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? – Anton Shepelev Jan 01 '18 at 21:41
  • I guess the OP had this coming by using result codes rather than exceptions... – JacquesB Jan 02 '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 02 '18 at 12:09
  • @JacquesB, I hope you agree that this kind of coupling is like a stack: the more we initialise at the start the more shall we have to deinitialise in the end, in the exact reverse order. Now, a stack is in fact a linear structure and is best represented linearly, e.g. via an array and a counter, which proves that there is no actual hierarchy in a stack. So I have proposed a linear solution to your problem. The need to add a new nesting level for each init-deinit pair will make your code unmanageable very soon. – Anton Shepelev Jan 02 '18 at 13:44
  • 1
    your code is a messier version of using exceptions – Austin_Anderson Jan 02 '18 at 13:59
  • I agree that nesting will get out of hand but I really dislike that you have removed resource safety, there are plenty of exceptions that might arise regardless, effectively writing more code to do less useful work. – Aluan Haddad Jan 02 '18 at 14:01
  • 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 02 '18 at 14:03
  • A single `Exception` and your whole stack will come crashing down. There are enough good ways to handle this without trying to homebrew it with `goto`. – nvoigt Jan 02 '18 at 15:28
  • The fact that goto is included in the language implies that people who have a lot of experience with software design recognize there are valid use cases for it, but I wouldn't expect not to encounter high resistance among your co-workers, boss, and the majority in forums like this. – Thomas Carlisle Jan 02 '18 at 16:09
  • @JacquesB, no function calls (call graph) comprise a true hierarchy, which cannot be expressed as a linear structure, stack or otherwise. Your overgeneralise. – Anton Shepelev Jan 03 '18 at 10:17
  • Can anyone help me to move this discussion into chat? – Anton Shepelev Jan 03 '18 at 10:21
  • @AluanHaddad, Since the *OP* was using return value instead of exceptions, I found it natural to assume that his subroutines were exception-free, e.g. they caught exceptions internally and reported them as error codes. – Anton Shepelev Jan 03 '18 at 10:24
  • @nvoigt: what are those "good ways" that you refer to? I do not see them among the answers to this question. Nor do I see any good in trying to emulate a linear structure of control flow with deeply nested `if`'s, `try`'s, or `using`'s – Anton Shepelev Jan 03 '18 at 10:42
  • @Ant_222 So if you don't want to use the proper language constructs *how do you* want to handle exceptions with your code? Ho *do* you make sure your Error: label is hit no matter what? Because right now as written, your code is faulty compared to the other solutions concerning exceptions. – nvoigt Jan 03 '18 at 10:58
  • @nvoigt: As I said, the use by the *OP* of return values as error indicators suggests that his functions do not let out any exceptions. This behavior is easily achieved by a) never throwing your own exceptions, b) catching all exceptions in third-party *API*'s as early as possible, and c) treating any exceptions that occur in your code as failed asserts or design errors. It as a *very bad idea* to mix error flags and exceptions together—one ought to choose one technique of error handling and and use it alone throughout the program. – Anton Shepelev Jan 03 '18 at 21:47
  • I hope everybody agrees the me that if a function be declared as returning a boolean error flag or some `Result` structure with detailed error information, it would be unpractical and even insane also to let it terminate with an exception in some cases. My solution therefore assumes exception-free functions. – Anton Shepelev Jan 03 '18 at 21:53
  • @Ant_222 I'm sure there are languages where this is an option, but C# does not have exception free functions. That is why it has those language constructs and that is why one should use them. – nvoigt Jan 04 '18 at 08:43
  • @Ant_222 I agree with you that it doesn't make sense to have a function that returns a Result structure throw exceptions for stuff that it can reasonably try/catch for itself. In C# we do not null check every reference type variable in every location because it would lead to bloated code. Its fine to assume those `Result` functions do not need to be try/caught at every level, assuming you have some kind of top-level try/catch/errorpage in your app that handles the rare and completely unexpected exceptions that can occur. – Graham Jan 04 '18 at 13:22
  • @nvoigt: Even in C# one can make a function exception-free by design. The easiest way to do so is to wrap its body in a `try..catch` block. But in higher-level functions that depend only on one's own exception-free functions it is not required. – Anton Shepelev Jan 04 '18 at 19:48