0

I have a requirement to have a component that will check input against some rules in a strict order. The order of the rules is defined by the business.

To keep example code simple, I'll try to define an analogy. Let's say business defines order of some int values. You'll be given sequence of int numbers and requirement is to check if sequence contains a number from the numbers defined by the business. If it does, return the number, if none of those numbers are contained in sequence, return -1. Checks should be made in order (this is important!).

For example: Let's say business defines order of numbers to be checked as {3, 7, 5}. When input integer sequence (example {4, 6, 7}) is received we should check in the defined order if number exists in received sequence. So in our example

  1. We check if number 3 exists in the sequence, clearly it doesn't
  2. Then we check if number 7 exists in the sequence, it does, so we return 7

As I said this is simplified analogy, the rules in real application are much more complicated, so is the input.

My implementation is making use of chain of rules pattern. There is a chain of rule, where each rule knows what's the next rule & upon checking input against it's logic, in case it's not satisfied, passes input to the next rule. Implementation for our numbers analogy

public interface INumberRule
{
  int GetNumber(IEnumerable<int> numbers);
}

INumberRule interface defines API for our rules.

public sealed class ChainedRule : INumberRule
    {
        private INumberRule NextRule { get; }
        private int Number { get; }

        public ChainedRule(INumberRule nextRule, int number)
        {
            NextRule = nextRule;
            Number = number;
        }

        public int GetNumber(IEnumerable<int> numbers)
        {
            if (numbers.Contains(Number))
                return Number;

            return NextRule.GetNumber(numbers);
        }
    }

ChainedRule defines the skeleton of our rule. All rules are going to be ChainedRule, except of chain termination rule.

public sealed class MinusOneRule: INumberRule
    {
        private MinusOneRule()
        { }

        public int GetNumber(IEnumerable<int> numbers) => -1;

        public static MinusOneRule Instance { get; } = new MinusOneRule();
    }

Sole purpose of MinusOneRule is to terminate the chain when none of the rules could satisfy their logic against input. As outlined above, if none of the numbers contained in the input sequence, we should return -1. MinusOneRule does just that.

We define factory that creates chain of rules:

public interface INumberRuleFactory
    {
        INumberRule GetChainedRules();
    }

    public sealed class ChainedNumberRuleFactory : INumberRuleFactory
    {
        public INumberRule GetChainedRules()
        {
            var minusOneRule = MinusOneRule.Instance;

            var sevenRule = new ChainedRule(minusOneRule, 7);

            var fiveRule = new ChainedRule(sevenRule, 5);

            var threeRule = new ChainedRule(fiveRule, 3);

            return threeRule;
        }
    }

And finally the usage:

public sealed class NumberProvider2
    {
        private INumberRule NumberRules { get; }
        public NumberProvider2(INumberRuleFactory numberRuleFactory)
        {
            NumberRules = numberRuleFactory.GetChainedRules();
        }

        public int GetNumber(IEnumerable<int> numbers) => NumberRules.GetNumber(numbers);
    }

class Program
    {
        static async Task Main(string[] args)
        {
            IEnumerable<int> numbers = new[] {4, 6, 7};
            var numberRulesFactory = new ChainedNumberRuleFactory();
            var numberProvider2 = new NumberProvider2(numberRulesFactory);
            WriteLine($"Number Provider v. 2: {numberProvider2.GetNumber(numbers)}");

            ReadLine();
        }
}

I like this approach because it is robust, it clearly communicates intention & importance of the order.

Colleague of mine suggested that this might be too much code and instead it could be done using sequence of rules. This sequence is also created by a factory & factory is responsible for properly ordered number rules.

public interface IOrderedNumberSequenceFactory
    {
        IEnumerable<int> GetOrderedNumbers();
    }

    public sealed class OrderedNumberSequenceFactory : IOrderedNumberSequenceFactory
    {
        public IEnumerable<int> GetOrderedNumbers() => new[] {3, 7, 5};
    }

And the usage:

public sealed class NumberProvider
    {
        private IEnumerable<int> OrderedNumbers { get; }
        public NumberProvider(IOrderedNumberSequenceFactory orderedNumberLisProvider)
        {
            OrderedNumbers = orderedNumberLisProvider.GetOrderedNumbers();
        }

        public int GetNumber(IEnumerable<int> numbers)
        {
            foreach (int orderedNumber in OrderedNumbers)
            {
                if (numbers.Contains(orderedNumber))
                    return orderedNumber;
            }

            return -1;
        }
    }

class Program
    {
        static async Task Main(string[] args)
        {
            IEnumerable<int> numbers = new[] {4, 6, 7};

            var rulesFactory = new OrderedNumberSequenceFactory();
            var numberProvider = new NumberProvider(rulesFactory);
            WriteLine($"Number Provider v. 1: {numberProvider.GetNumber(numbers)}");
        }
    }

I clearly can see this approach is less code, less components and easier to read & understand at first glance. However, what I don't like is it doesn't seem to communicate strongly the importance of rule order & it is easier to mess order with this implementation than with the previous one.

The opposite argument was if factory method that creates ordered numbers is properly named (e.g. GetOrderedNumbers()), then it is clearly communicating that order is important here.

Any thoughts around those approaches with pros/cons ?

  • 1
    "return -1" You will have a lot of fun when the rules change and -1 becomes a valid result... – Bernhard Hiller Jul 18 '18 at 7:48
  • @BernhardHiller: I agree with what you're trying to point out, but keep in mind that (1) OP is intentionally using a dumbed down example and (2) Methods like IndexOf do still rely on -1 values because the context of the method makes it clear that -1 will always be a nonsensical value. The same might apply to OP's real use case. – Flater Jul 18 '18 at 7:54
  • Both of these API's look clumsy to me. The only interesting part about the masses of code is that they both contain a sequence of numbers. Everything seems to be noise and over engineering. – Frank Hileman Jul 19 '18 at 0:10
4

Linked lists are outdated.

What you essentially have here is a linked list of validation steps.

Linked lists are an interesting lesson on data structures if you're learning about reference types. And in the days of C/C++, linked lists also provided a valuable feature: a collection whose size does not need to be defined in advance (as opposed to arrays).

However, in today's C#, I see no benefit to linked lists anymore, since the advent of List<T>. Lists provide the same functionalities (ordered collection, can grow organically) and don't require you to develop the glue that holds the collection together.

There is still an interesting discussion to be had about List versus LinkedList (mostly in the comments to the accepted answer), but that's a different discussion. You're using a manually built linked list, not a LinkedList.

Note that when I say linked lists are outdated, I mean manually crafted linked lists like yours. As mentioned in the comments here, the benefit of linked lists is that you can insert items in the middle of the list in O(1) time.
However, that can already be achieved by using the LinkedList class; there's still no reason to manually craft classes to act as linked list nodes.


Your solution.

I like this approach because it is robust, it clearly communicates intention & importance of the order.

I strongly oppose the claim that it communicates the importance of the order. Your code has inverted the order of rules, starting with the lowest priority and building up to the highest priority.

As a matter of code readability, your code is exactly opposite of what you'd expect: the top priority rule (at the bottom) is an early exit clause and would instinctively be found at the top of the code.

Your code should be an generalization of a pattern that looks something like:

if(value == first_number) 
    return first_number;

if(value == second_number) 
    return second_number;

return default_number;

This is just a simple pseudocode example, but this pattern of early returns is incredibly common to encounter in code. And in all common cases, the highest priority is found at the top, not the bottom.

I also think you're confusing "clearly communicating intention" with "it does what I had already decided it should do". By itself, without reading your explanation (which is what I always do first to test code readability), it was not clear what you tried to achieve. The essence of your solution is hidden in a lot of structural boilerplated code.
Tongue-in-cheek, one man's "robust" is the other man's "overengineered".

If anything, your intention can be summed up by two tiny snippets:

if (numbers.Contains(Number))
            return Number;

var threeRule = ...
var fiveRule = ...
var sevenRule = ...
var minusOneRule = ...;

Everything else is just dressing. As an interesting point, I inverted the order of your rule variables. Because without relying on the notion that you're creating a chained structure, the inverted order of variables leads an errant reader to assume that the order of rules is the other way around (-1 first).


Your colleague's solution.

However, what I don't like is it doesn't seem to communicate strongly the importance of rule order

I disagree. Your colleague's code makes more sense; the validation numbers are described in descending order of priority, which is what you'd instinctively expect.

I also very much aprove that his code separated the intended values from the default value. -1 is not an equal to 3, 7 or 5. It is functionally different. If your application needs to support negative numbers in the next version, you need a way to change your default value without affecting the rest of your code.

& it is easier to mess order with this implementation than with the previous one.

How?

If you're liable to mistake the order of the numbers in you're colleague's list, you're just as liable to mistake the order of the elements in your chained rules.

I've mentioned it before, but the inverted order of defining your rules, if anything, will make it more likely that mistakes happen when the rules get redefined.

  • Thank you very much for this detailed answer ! Makes perfect sense. – Michael Jul 18 '18 at 9:20
  • I think you are going too far to say linkedlist has no benefit anymore. the obvious one that springs to mind is that linked lists have O(1) insert at any place in the list – jk. Jul 18 '18 at 11:09
  • @jk. I should've specified manually crafted linked lists, as is the case in OP's scenario. LinkedList provides the O(1) you want. – Flater Jul 18 '18 at 11:33

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.