1

In my application I'm dealing with periods. A period is defined by it's end date. A period may shrink or grow, can restart and can have a priority. This is modeled using the following struct:

public struct Period
{
    public DateTime EndDate { get; set; }
    public bool CanShrink { get; set; }
    public bool CanGrow { get; set; }
    public bool Restart { get; set; }
    public int Priority { get; set; }
}

I have a business object which has such a period. During program execution there can be a new period which might be applied to the business object following a set of rules depending on which period is shorter and which can shrink or grow. Here's some of the rules for an existing period which is shorter than the new period:

existing can shrink existing can grow new can shrink new can grow new restarts action
0 0 0 0 0 Apply period with higher priority
0 0 0 0 1 Apply new period
0 0 0 1 0 Apply period with higher priority
0 0 0 1 1 Apply new period
0 0 1 0 0 No action required
0 0 1 0 1 Apply new period
0 0 1 1 0 No action required
0 0 1 1 1 Apply new period

In total there are 64 possible combinations (32 for each flag in the table, one table if the existing period is shorter and one table if the existing period is longer). I could model this with a bunch of if/else's. Then I had the idea to implement the tables as dictionaries of delegates:

private readonly Dictionary<(bool, bool, bool, bool, bool), Action<Period, Period>> existingPeriodBeforeNewPeriodActions;
private readonly Dictionary<(bool, bool, bool, bool, bool), Action<Period, Period>> existingPeriodAfterNewPeriodActions;

This would be how the above table will be represented:

existingPeriodBeforeNewPeriodActions =
    new Dictionary<(bool existingCanShrink, bool existingCanGrow, bool newCanShrink, bool newCanGrow, bool restart), Action<Period, Period>>
    {
        { (false, false, false, false, false), (e, n) => ApplyPrioritizedPeriod(e, n) },
        { (false, false, false, false, true), (e, n) => ApplyPeriod(n) },
        { (false, false, false, true, false), (e, n) => ApplyPrioritizedPeriod(e, n) },
        { (false, false, false, true, true), (e, n) => ApplyPeriod(n) },
        { (false, false, true, false, false), (e, n) => { /* NOOP */ } },
        { (false, false, true, false, true), (e, n) => ApplyPeriod(n) },
        { (false, false, true, true, false), (e, n) => { /* NOOP */ } },
        { (false, false, true, true, true), (e, n) => ApplyPeriod(n) },
        /* 24 more entries */
    };

The method which applies the new period to the business object would then use this dictionaries so:

public void UpdatePeriod(Period existingPeriod, Period newPeriod)
{
    // Periods end on same day => no updated required   
    if (existingPeriod.EndDate == newPeriod.EndDate)
        return;

    Action<Period> updateAction;
    if (existingPeriod.EndDate < newPeriod.EndDate)
    {
        updateAction = existingPeriodBeforeNewPeriodActions
        [
            (
                existingCanShrink: existingPeriod.CanShrink,
                existingCanGrow: existingPeriod.CanGrow,
                newCanShrink: newPeriod.CanShrink,
                newCanGrow: newPeriod.CanGrow,
                restart: newPeriod.Restart
            )
        ];
    }
    {
        updateAction = existingPeriodAfterNewPeriodActions
        [
            (
                existingCanShrink: existingPeriod.CanShrink,
                existingCanGrow: existingPeriod.CanGrow,
                newCanShrink: newPeriod.CanShrink,
                newCanGrow: newPeriod.CanGrow,
                restart: newPeriod.Restart
            )
        ];
    }
    updateAction(newPeriod);
}

Is this a good approach regarding readability and maintainability? Speed is not so much of a concern here.

2 Answers 2

3

To be honest, I would not have the foggiest clue how this code maps to business rules. While you can use a dictionary and Actions to implement the behavior, I would have trouble tracing through the code to understand what it does. I encounter very general names for things. This would require me to see the source code for each action in order to understand conceptually what it does.

This is where a good name clears things up.

Conceptually what you have is a decision and a resulting action. Going bigger picture, you have multiples kinds of decisions to make. Given the state of an object, multiple actions might apply (or not depending on business rules). It feels like this decision/action combo is a missing abstraction. In fact, the action column in your question sounds like a great place to start naming things and coupling the decision to act with the action itself:

public interface IPeriodAction
{
    bool IsApplicable(Period existingPeriod, Period newPeriod);
    void Apply(YourBusinessObject foo, Period newPeriod);
    // or void Apply(Period newPeriod)
}

It's a little unclear how much access these actions need to your business class, so you'll need to find the right balance of exposing data versus encapsulating it, but this interface gives you the ability to build a list of actions.

Then, taking your first action, Apply period with higher priority, name a class that serves as the single source of truth for when this logic should be applied:

public class ApplyHigherPriorityPeriodAction : IPeriodAction
{
    public bool IsApplicable(Period existingPeriod, Period newPeriod)
    {
        return !existingPeriod.CanShrink
            && !existingPeriod.CanGrow
            && !newPeriod.CanShrink
            && !newPeriod.CanGrow;
    }

    public void Apply(YourBusinessObject foo, Period newPeriod)
    {
        // logic goes here
    }

    // Or...
    //public void Apply(Period newPeriod)
    //{
    //    // logic goes here
    //}
}

The code is immediately clear, and easy to unit test. You also establish a naming convention making extending this logic easier.

You can build a list of these actions in the order you want them processed, and still process them in bulk:

var actions = new List<IPeriodAction>()
{
    new ApplyHigherPriorityPeriodAction(),
    new ApplyNewPeriodAction(),
    new NoActionRequiredPeriodAction()
};

// Or .Single(...) or .Where(...) - whatever suits your business use case.
var actionToExecute = actions.First(a => a.IsApplicable(existingPeriod, newPeriod));

actionToExecute.Apply(...);
1

Is this a good approach regarding readability and maintainability?

Meh? It can be a fine approach, though for this it seems to be a step down. Five bools to an action doesn't have a particularly intuitive expectation of behavior. Someone coming into this code fresh will see it and have to figure out what the hell is going on.

And this sort of thing can be good when the signature of the delegates doesn't change (when the actual business logic is the only thing that varies). Here, the most likely change is the various bools as business logic expands to consider other situations/operations than grow, shrink, existing, and new. That signature change will touch all of the declarations that use it, which tends to be a lot bigger pain in the ass than editing a big if/else block.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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