5

I'm working on an interface for implementing business rules in order to improve SOLID-ity; so I can move a lot of logic out of Web API controllers and into a business library. The Common Problem being that an action should occur if one or several conditions are met, and some of these conditions are likely to be required throughout the system with different actions as the end result. I've done some research and came up with the code below. Does this conform to an existing design pattern? I looked in the GoF list and didn't find any matches there.

/// <summary>
/// Use for designing a business rule where conditions are evaluated and the actions are executed based on the evaluation.
/// Rules can be chained by setting the "Action" as another business rule.
/// </summary>
/// <typeparam name="TCondition">The type of the condition or conditions.</typeparam>
/// <typeparam name="TAction">The type of the action or actions to be executed.</typeparam>
/// <typeparam name="TResult">The type of the result.</typeparam>
/// <seealso cref="Core.Interfaces.IBusinessRule" />
internal interface IBusinessRule<TCondition, TAction, TResult> : IBusinessRule
    where TCondition : IRulePredicate where TAction : IRuleAction<TResult>
{
    ICollection<TAction> Actions { get; set; }

    ICollection<TCondition> Preconditions { get; set; }
}


internal interface IBusinessRule
{
    IEnumerable Results { get; }

    RuleState State { get; }

    Task Execute();
}

public enum RuleState
{
    None,
    Initialized,
    InProgress,
    Faulted,
    FailedConditions,
    Completed
}

public interface IRulePredicate
{
    bool Evaluate();
}

public interface IRuleAction<TResult>
{
    Task<TResult> Execute();
}


public abstract class RuleBase<TCondition, TAction, TResult> :
    IBusinessRule<TCondition, TAction, TResult> where TCondition : IRulePredicate
    where TAction : IRuleAction<TResult>
{
    public ICollection<TResult> Results { get; } = new List<TResult>();

    public ICollection<TCondition> Preconditions { get; set; } = new List<TCondition>();

    public ICollection<TAction> Actions { get; set; } = new List<TAction>();

    IEnumerable IBusinessRule.Results => Results;

    public RuleState State { get; private set; } = RuleState.Initialized;

    public async Task Execute()
    {
        State = RuleState.InProgress;
        try
        {
            var isValid = true;
            foreach (var item in Preconditions)
            {
                isValid &= item.Evaluate();
                if (!isValid)
                {
                    State = RuleState.FailedConditions;
                    return;
                }
            }

            foreach (var item in Actions)
            {
                var result = await item.Execute();
                Results.Add(result);
            }
        }
        catch (Exception)
        {
            State = RuleState.Faulted;
            throw;
        }

        State = RuleState.Completed;
    }
}

public class TestRule1 : RuleBase<FakePredicateAlwaysReturnsTrue, WriteHelloAction, string>
{
    public TestRule1()
    {
        Preconditions = new[] { new FakePredicateAlwaysReturnsTrue() };
        Actions = new[] { new WriteHelloAction() };
    }
}

public class FakePredicateAlwaysReturnsTrue : IRulePredicate
{
    public bool Evaluate()
    {
        return true;
    }
}

public class WriteHelloAction : IRuleAction<string>
{
    public async Task<string> Execute()
    {
        return await Task.Run(() => "hello world!");
    }
}


public static class Program
{
    public static async Task Main()
    {
        IBusinessRule rule = null;

        try
        {
            rule = new TestRule1();
            await rule.Execute();

            foreach (string item in rule.Results)
            {
                // Prints "hello world!"
                Console.WriteLine(item);
            }
        }
        catch (Exception ex)
        {
            if (rule != null && rule.State == RuleState.Faulted)
            {
                throw new Exception("Error in rule execution", ex);
            }

            throw;
        }
    }
}

migrated from codereview.stackexchange.com Jun 23 '16 at 4:03

This question came from our site for peer programmer code reviews.

  • 1
    I would call it way too complicated and breaks SRP. – Euphoric Jun 23 '16 at 4:24
  • 1
    I would post it back to codereview and ask them to review it instead of asking if it has name (it doesn't, because only simple things have names). I don't understand why it was migrated here, because that code is more in need of critique than naming. – Euphoric Jun 23 '16 at 4:33
  • @Euphoric Code Review kicked me out. They said the question was off-topic. – lorddev Jun 23 '16 at 4:38
  • 5
    @Euphoric The question was borderline on Code Review. We tried to keep it an open-ended request for critique as required by Code Review's topicality rules, but the author insisted on a title and question body that was more about naming the design pattern, and wanted to accept Dmitry's answer, which is not a code review. We mutually agreed to migrate the question, as originally asked, to Programmers. – 200_success Jun 23 '16 at 5:58
  • 1
    @200_success Fair enough. lorddev, patterns are often simple solutions to common problems. Your code is neither simple nor for common problem. I would really not try to find some generally agreed name to fit it into. – Euphoric Jun 23 '16 at 6:01
9

You could probably have a look at Rules Design Pattern. There is a good video also at Pluralsight, see Rules Pattern (you will need to sign in).

  • -1 No code snippet or examples. – lKashef Jul 4 at 20:17
11

To expand on my comment I will give you my opinion on your code :

Generics : I believe there is thin line between cases where generics are useful and cases where they are abused and only bring more problems. Yours is way past into the problems area. Just looking at the big generic definition rings an alarm for me. This is augmented by fact that you have both non-generic and generic interfaces for the same thing. This could bring your problems with composition of the business rules. I would look into refactoring it into just interfaces.

Error handling : You are using both "faulted" state and exceptions. This is weird and confusing. Either use one or other. I would go for RuleFaultedException and throw that instead of setting a faulted state. This makes both the business rule and the calling code simpler.

Separation of precondition and actions : To me, an action and precondition if that action can be executed are cohesive parts. They should be together and inseparable. If some business rule has same action and different preconditions, then that rule should be multiple rules. Breaking both up, like in your case, breaks SRP (contrary to what most people believe SRP works both ways, it separates non-cohesive behavior and groups cohesive behavior).

Exposing modifiable collections : Your business rule class exposes a ICollection, which is modifiable. This means, that after concrete rule is created, the collection can be added into. It might not be what is desirable with specific business rules. While it is true that in your case, if you assign array into the property, that will result in runtime exception. It is still true that the interface is not correctly exposing what is possible with it through types. You could replace it with IEnumerable without much problems.

Multiple actions and results : How often do you see business rule with multiple actions that has multiple results? If not that often, then having multiple actions and results needlessly complicates the calling code, because calling code always need to assume you get multiple results. This makes the code more complicated that it usually needs to be. Your design also makes it really hard to put multiple actions into rule with actions having different return types. You can always return object, but type erasure is never a good thing when exposed to the calling code.

  • Thanks very much for the Code Review™. I wanted to chain the preconditions so that if any of them were unmet the Action wouldn't fire. That's my chief goal. So I should make the Action singular. And I'll implement the exception recommendation and make the collection read-only. Thanks! – lorddev Jun 23 '16 at 6:07
  • 1
    @lorddev I think the preconditions/actions are the most critical problem in your design. I would instead design IBusinessRule that has both precondition and action. And then use decorator to compose actions with different preconditions. – Euphoric Jun 23 '16 at 6:18
2

For the most part rule Preconditions is validation. With the approach of keeping validation outside of Rule action and execution flow you will get:

  1. You will be able to reuse Preconditions. - In most cases it's a useless flexibility. Moreover usually you will have rules only with one precondition and one rule action.
  2. There will be a problem of sharing data between preconditions and rule actions. Usually they need common context of execution, like you validate let's say age, then you write age to the database. Passing age to precondition and action constructors would be too much work (what if there are 10 parameters).

What I would try to achieve:

  1. Keep it simple for developers: easy to understand / easy to implement.
  2. Keep it testable.
  3. Granulate business logic and move it out of the controllers.

I would try to create a simple implementation of CQRS. Where Q is repository pattern implementation and C is something trivial like:

public interface ICommand 
{
    Task ExecuteAsync();
}

And if needed it implements:

public interface IResultable<TResult>
{
    TResult Result { get; set; }
}

All business logic and validation is inside ExecuteAsync. Some stuff can be moved to a separate classes if it gets to big.

Web API usually needs to catch a few types of exceptions to wrap it in NotFound or BadRequest HTTP results. For this case you need to introduce Core exceptions and catch them in Exception handling filter. All the other exceptions need to be wrapped in InternalServerError HTTP code.

  • Thanks for the ideas. I'm not intending to use this for validation though, but business rules. For example, if client is configured for billing and user has billing then execute billing action . – lorddev Jun 23 '16 at 17:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.