CODEWITHSUNDEEP
c#algorithmdesign-patternsmaintainabilitycode-readability
Tony_KiloPapaMikeGolf
Tony_KiloPapaMikeGolf

Reputation: 889

Best design pattern for structured sequential handling

Doing maintenance on a project I came across code, which I find unnecessary hard to read and I wish to refactor, to improve readability.

The functionality is a long chain of actions that need to be performed sequentially. The next action should only be handled if the previous action was successful. If an action is not successful a corresponding message needs to be set. And the returned type is a Boolean. (successful true/false). Just like the return type of all the called actions.

Basically it comes down to something like this.

string m = String.Empty; // This is the (error)Message.
bool x = true; // By default the result is succesful.

x = a1();
if(x) {
    x = a2();
}
else {
    m = "message of failure a1";
    return x;
}

if(x) {
    x = a3();
}
else {
    m = "message of failure a2";
    return x;
}

//etcetera..etcetera...

if(x){
    m = "Success...";
}
else{
    m = "Failure...";
}

return x;

My question is: What is a better structure / pattern to handle this kind of logic?

Main goals are:

Please keep in mind that it is quite a large chain of actions that is being performed sequentially. (Thousands lines of code)

Upvotes: 2

Views: 2298

Answers (2)

Sergey Kalinichenko
Sergey Kalinichenko

Reputation: 726569

Make a list of action/message pairs:

class Activity {
    public Func<bool> Action { get; set; }
    public String FailureMessage { get; set; }
}

Activity[] actionChain = new[] {
    new Activity { Action = A1, FaulureMessage = "a1 failed"}
,   new Activity { Action = A2, FaulureMessage = "a2 failed"}
,   new Activity { Action = A3, FaulureMessage = "a3 failed"}
};

A1..A3 are no-argument methods returning bool. If some of your actions take parameters, you can use lambda expressions for them:

Activity[] actionChain = new[] {
    ...
,   new Activity { Action = () => An(arg1, arg2), FaulureMessage = "aN failed"}
,   ...
};

Now you can go through the pairs, and stop at the first failure:

foreach (var a in actionChain) {
    if (!a.Action()) {
        m = a.FailureMessage;
        return false;
    }
}
m = "Success";
return true;

Upvotes: 5

JanDotNet
JanDotNet

Reputation: 4016

@dasblinkenlight was faster... however a similar solution using yield instead of an array:

public string Evaluate()
{
    foreach (var customAction in EnumerateActions())
    {
        if (!customAction.Execute())
            return customAction.Error;
    }
    return "Success...";
}

public IEnumerable<CustomAction> EnumerateActions()
{
    yield return new CustomAction(a1, "Error for A1");
    yield return new CustomAction(a2, "Error for A2");
    ...
}

public class CustomAction
{
    public Func<bool> Execute { get; }
    public string Error { get; }
    public CustomAction(Func<bool> action, string error)
    {
        Execute = action;
        Error = error;
    }
}

Upvotes: 4

Related Questions