DRY Fix Possibilities, C#

Question

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?

Answer 1

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

Answer 2

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>}

Answer 3

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.

Answer 4

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

Answer 5

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 :)