4

This code has been bothering me all day, partially because the

if (result != OpResult.Success) { // return

code is repeated, everywhere.

A series (1..n) of evaluation are performed. After each evaluation, a check is made to ensure that the operation was a success (utilizing a custom return value derived from an enumeration): OpResult.Success.

Here is an example (with example objects, etc.):

OpResult result = OpResult.Sucess;

result = performOperationOne(commonObjectArgument);

if (result != OpResult.Success)
{
    trace.Exit(); // Exit logging mechanism
    return result;
}

result = performOperationTwo(commonObjectArgument);

if (result != OpResult.Success)
{
    trace.Exit();
    return result;
}

As you can see, if (result != OpResult.Success) is used as flow control, i.e. unless all preceeding oprations are a success, then the next will not run.

With the .Net 4.*, C# has become capable of some pretty incredible things, syntactically. Is there anything that I can do to eliminate needing to re-write this evaluation after every operation?

Thomas
  • 6,291
  • 6
  • 40
  • 69
  • Unfortunately, without knowing how `trace` is implemented, it's hard to tell. – Justin Niessner Mar 21 '16 at 20:22
  • @JustinNiessner if I remove trace (or, here, treat it as a simple operation), does that open up some possibilities? It's just writing out to a file using `System.Diagnostics.DefaultTraceListener` – Thomas Mar 21 '16 at 20:23

5 Answers5

9

One possibility is to build a list of operations and perform them in a loop:

var operations = new List<Func<CommonObjectArgumentType, OpResult>>
{
    Operation1,
    Operation2
};

OpResult result = OpResult.Success;
foreach (var op in operations)
{
    result = op(commonObjectArgument);
    if (result != OpResult.Success)
    {
        trace.exit();
        return result;
    }
}

// all operations were successful
Jim Mischel
  • 131,090
  • 20
  • 188
  • 351
  • This is really good answer and a powerful technique! It gives a solution for the people searching for a way to avoid multiple return statements while keeping the the code very concise and readable. (one can use a `List>` in order to avoid multiple disjunctive `if` clauses with `return` statements). Thank you very much! – Alexander Mihailov Jun 19 '19 at 08:28
5

If the signatures are the same, as suggested by your sample code, you could execute those functions in a loop:

// create a collection of your functions where `object` is your argument type
var functions = new Func<object, OpResult>[] {
    performOperationOne,
    performOperationTwo,
    /* etc... */
};

var result = OpResult.Success;

foreach(var function in functions)
{
    result = function(commonObjectArgument);
    if (result != OpResult.Success)
    {
        trace.Exit(); // Exit logging mechanism
        break;
    }
}

return result;

That way all your status checking is done in the same place.

Check out Func<T,TResult>

canon
  • 40,609
  • 10
  • 73
  • 97
  • 1
    It's kind of scary how closely our answers match, considering we wrote them independently and posted within a minute of each other. – Jim Mischel Mar 21 '16 at 20:40
  • @Thomas looks like, Jim beat me. – canon Mar 21 '16 at 20:42
  • 1
    @canon thanks for the help! I am sorry that I can't accept both! – Thomas Mar 21 '16 at 20:43
  • 1
    @Thomas: Flip a coin? We posted nearly the identical answer within a minute of each other, and we each upvoted the other. If it answers your question, then upvote them both and select whichever one you like as the accepted answer. – Jim Mischel Mar 21 '16 at 20:43
  • 1
    @JimMischel upvoted both, thank you! You won the toss. :-) – Thomas Mar 21 '16 at 20:44
1

If you don't want to change the methods, you might try to array your function calls such that you can do...

var success 
  ( doFunctionOne() == OpResult.Success ) &&
  ( doFunctionTwo() == OpResult.Success ) &&
  ( doFunctionThree() == OpResult.Success );

...or something to that effect. It short circuits so that the subsequent doFunctionX calls don't execute after one of 'em fails.

Clay
  • 4,999
  • 1
  • 28
  • 45
0

Since you asked about fancy syntax, you could use LINQ to Objects:

var operations = new List<Func<object, OpResult>> {
    performOperationOne,
    performOperationTwo,
    // ...
};

var failures = from operation in operations
               let result = operation(commonObjectArgument)
               where result != OpResult.Success
               select result;

// `failures` is lazy-evaluated, so nothing will be executed until we begin this loop
foreach (var failure in failures) {
    trace.Exit();
    return failure;
}

// If we reach this point, we know there were no non-`Success` results
Jacob Krall
  • 28,341
  • 6
  • 66
  • 76
-1

Multiple returns are generally frowned upon from a coding standards point of view. A value should be assigned to result and returned at the closing of the method. The trace mechanism may be able to be called once only at the end regardless or conditionally (depending on the logic above). It's a matter of writing code in a better way here. Without knowing the functionality of trace, then it's not possible to advise best way forward regard its usage. Regardless though, it should be implemented at the closing of the method along with the returned value.

You don't require a fancy syntactical solution to this. Just plain old good coding practice :)

ManoDestra
  • 6,325
  • 6
  • 26
  • 50
  • 4
    A highly rated wiki article http://stackoverflow.com/questions/36707/should-a-function-have-only-one-return-statement on this site seems to disagree with your characterization of multiple returns being frowned upon. – Michael Mar 21 '16 at 20:42
  • 1
    @Michael It's very common practice in Javascript, certainly, and a great way to exit a function for ease of readability when needed. – Thomas Mar 21 '16 at 20:45
  • @Michael I think the aversion to it has more to do with [branch prediction](https://en.wikipedia.org/wiki/Branch_predictor) than with readability. I don't know if it's still the case, but it used to be best practice to structure your if statements such that the expected condition tested positively. – canon Mar 21 '16 at 20:48
  • This isn't JavaScript though. It's C#. And it's generally frowned upon. It doesn't mean you can't do it, of course, but it's generally better to write code that doesn't do multiple returns as it can make code far less readable and debuggable. If there's only one return at the top that exits in the event of some validation failure, say, then returns at the bottom, fine, but tons of returns throughout a method can be easily rewritten to ensure a single point of return that makes it far harder for values to escape the method without being managed correctly in code. – ManoDestra Mar 21 '16 at 20:49
  • Agreed @canon. It's just as easy to write code correctly in the first place with a single return than the lazy multiple return method that can easily cause maintainability issues later. http://stackoverflow.com/questions/12685675/is-it-good-practice-use-more-that-one-return-statement-in-a-method – ManoDestra Mar 21 '16 at 20:50
  • 3
    @ManoDestra _tons of returns throughout a method can be easily rewritten to ensure a single point of return_ ... and the trade off here is _Christmas tree-ifying_ your code slightly. – Michael Mar 21 '16 at 20:54
  • Not, if you factor your code appropriately. If you're getting too bogged down into indented nested, if, while or for constructs, then you're not likely to have coded the method correctly in the first place. Each method should be small and do one thing simply. Or be a list of calls to other methods that do something simply. It's all about how you write and structure your code. And a single return is a far superior methodology (with some minor concessions to this general rule). And if the code in the OP's question were written appropriately, then he would not even have the issue he reports here. – ManoDestra Mar 22 '16 at 19:26
  • 1
    @ManoDestra that is certainly a good point. I agree with your latter statement, as well: "if the OP's [code] were written appropriately" -- I think as engineers, though, we often encounter points where we don't have the architecture control we would wish and are left with some pretty crappy methods to refactor. – Thomas Dec 16 '16 at 20:37