calling methods if previous call success

Question

I my c# program I have to perform 5 steps(tasks) sequentially. basically these five should execute one after the other only the previous task performed is success. Currently I have done it in following style. But this is not very good code style to follow.

var isSuccess=false;
isSuccess=a.method1();

if(isSuccess)
    isSuccess=a.method2();

if(isSuccess)
    isSuccess=a.method3();

if(isSuccess)
    isSuccess=a.method4();

if(isSuccess)
    isSuccess=a.method5();

How can I re factor this code. What is the best way I can follow?

The biggest question for me is what the point of these methods might be. If this is just for checking conditions, `&&` might be a good choice. Otherwise, you're just using return codes and should probably rethink the structure of your code, because this is a very procedural way to go about it. — Magus, Jun 10 '14 at 16:18

score 8 · Answer 1 · answered Jun 08 '14 at 20:46

C#/.NET process conditions left to right and would stop at the first false if they were "ANDed" together, e.g.

bool success = A() && B() && C();

should stop at B() if it returned false. Thus the following code will only output "In A" and "In B", but not "In C", because as soon as B() returns false, it stops.

using System;

public class App
{
  public static void Main()
  {
    bool success = A() && B() && C();
  }

  static bool A()
  {
    Console.WriteLine("In A");
    return true;
  }

  static bool B()
  {
    Console.WriteLine("In B");
    return false;
  }

  static bool C()
  {
    Console.WriteLine("In C");
    return true;
  }
}

Output:

In A
In B

score 2 · Answer 2 · answered Jun 08 '14 at 18:32

I would actually let it stay as it is, since this way it's easily readable and understandeable by anyone. But if you use the same pattern in a lot of places and all the method have the same signature here's an (overengineered and probably twisted) idea:

public class SequentialExecutor
{
    private Func<bool>[] functions;

    public SequentialExecutor(params Func<bool>[] functions)
    {
        this.functions = functions;
    }

    public bool Execute()
    {
        foreach (var func in functions)
        {
            bool isSuccess = func();
            if (!isSuccess)
            {
                return false;
            }
        }

        return true;
    }
}

You can use it like this:

var executor = new SequentialExecutor(a.method1, a.method2, a.method3, a.method1, a.method4);
bool isSuccess = executor.Execute();

P.S. NOT TESTED

I actually quite like this answer, though I'd make it static and pass the functions to execute rather than use a constructor. — Magus, Jun 10 '14 at 16:12

score 1 · Answer 3 · answered Jun 08 '14 at 19:22

1

This is the sort of situation where throwing an exception on failure may be neater. If that's your API, you can then do:

try {
    a.method1();
    a.method2();
    a.method3();
    a.method4();
    a.method5();
} catch (UnsatisifedException e) {
    // Some sort of recovery logic here...
}

answered Jun 08 '14 at 19:22

Donal Fellows

6,347
25
35

That's the way I'd do it in Java for sure; I'm less certain about C# idioms, but it is at least clearer and easier to write given that C# has proper exceptions in the first place. – Donal Fellows Jun 08 '14 at 19:23
Exceptions are expensive. Even with fast object creation, it'll be slower to throw an exception than evaluate a Boolean – DougM Jun 08 '14 at 21:13
1

More than the expense, this sounds like using exceptions for flow control, which is generally frowned upon (with good reason!). – Magus Jun 10 '14 at 16:14

calling methods if previous call success

3 Answers3