I'm writing a system that's responsible for changing Status of Order. There's a specific set of rules that define route of changing Statuses. But, Order and its Products have to satisfy some requirements associated to given Status.

For example, Let's say we have this route of status:

NEW -> ACCEPTED -> CLOSED

Status NEW requires e.g CustomerID to be filled

Status ACCEPTED requires: Customer ID to be filled + some other info

Status CLOSEDD requires: Customer ID, some other info and some other info2

I came with this solution which reduces amount of ifs by huge amount, but I think it's kinda over engineered in some parts, especially throwing previous requirements in list constructor
new List>(status_predicates[Statuses.Status_Creating])

The method named Predicate is just a helper to minimize amount of syntax.

I just realized that this:

new List<CustomPredicate<Order>>(status_predicates[Statuses.Status_Creating])

isn't going to work, so implementation changes a little bit (copying-pasting predicates from prev. status), but the concept stays the same.

using System;

using System.Collections.Generic;

using System.Linq;

using System.Text;



namespace Services

{

    public class CustomPredicate<T>

    {

        public Func<T, bool> Predicate { get; set; }

        public string Error { get; set; }



        public CustomPredicate(Func<T, bool> predicate, string error)

        {

            Predicate = predicate;

            Error = error;

        }

    }



    public class StatusService

    {

        // dictionary of possible status change route

        public static Dictionary<string, List<string>> status_routes = new Dictionary<string, List<string>>

        {

            { Statuses.Status_Creating, new List<string> { Statuses.Status_SentNotConfirmed } },

            { Statuses.Status_SentNotConfirmed, new List<string> { Statuses.Status_Accepted, Statuses.Rejected } },

            { Statuses.Status_Accepted, new List<string> { Statuses.InProgress } }

        };



        // Dictionary of Predicates that have to be satisfied in order to "upgrade" status

        // Every predicate list contains requirements of previous

        // list of requirements (look at list constructor)

        public static Dictionary<string, List<CustomPredicate<Order>>> status_predicates = new Dictionary<string, List<CustomPredicate<Order>>>

        {

            {

                Statuses.Status_Creating, 

                new List<CustomPredicate<Order>>

                {

                    Predicate(x => x.Products.All(c => !string.IsNullOrWhiteSpace(c.CustomerNote)), "Every product of this order needs to have an customer note field filled."),

                    Predicate(x => x.Company != null, "You need to pick a company that will provide those products")

                }

            },

            {

                // In constructor there's a list of previous predicates

                Statuses.Status_SentNotConfirmed,

                new List<CustomPredicate<Order>>

                {

                    Predicate(x => x.Products.All(c => !string.IsNullOrWhiteSpace(c.CustomerNote)), "Every product of this order needs to have an customer note field filled."),

                    Predicate(x => x.Company != null, "You need to pick a company that will provide those products")



                    Predicate(x => x.Products.All(c => !string.IsNullOrWhiteSpace(c.SomeProperty)), "Someproperty has no value"),

                    Predicate(x => x.Products.All(c => !string.IsNullOrWhiteSpace(c.SomeProperty2)), "Someproperty2 has no value"),

                    Predicate(x => !string.IsNullOrWhiteSpace(x.ContactInfo), "Contact info has to be filled"),              

                }

            },      

            {

                // In constructor there's a list of previous predicates

                Statuses.InProgress, 

                new List<CustomPredicate<Order>>

                {

                    Predicate(x => x.Products.All(c => !string.IsNullOrWhiteSpace(c.CustomerNote)), "Every product of this order needs to have an customer note field filled."),

                    Predicate(x => x.Company != null, "You need to pick a company that will provide those products")



                    Predicate(x => x.Products.All(c => !string.IsNullOrWhiteSpace(c.SomeProperty)), "Someproperty has no value"),

                    Predicate(x => x.Products.All(c => !string.IsNullOrWhiteSpace(c.SomeProperty2)), "Someproperty2 has no value"),

                    Predicate(x => !string.IsNullOrWhiteSpace(x.ContactInfo), "Contact info has to be filled"), 



                    Predicate(x => x.Products.All(c => hasArrived()), "All Products still hasn't arrived yet."),         

                }

            },

        };



        public static (bool Satisfies, List<string> Errors) CanStatusOfThisOrderBeChanged(Order Order, string targetStatus)

        {

            if (Order == null || string.IsNullOrEmpty(targetStatus))

            {

                return (false, new List<string> { "Order or status does not exists" });

            }



            var errors = new List<string>();



            // here's the validator:

            foreach (var CustomPredicate in status_predicates[targetStatus])

            {

                if (!CustomPredicate.Predicate.Invoke(Order))

                {

                    errors.Add(CustomPredicate.Error);

                }

            }



            return (!errors.Any(), errors);

        }



        public static List<string> GetAvaliableStatusesForThisOrder(Order ord)

        {

            if (!status_routes.ContainsKey(ord?.Status))

            {

                throw new Exception("Status does not exists");

            }



            return status_routes[ord?.Status]; // todo: checking predicates, but that's not the point of this question

        }



        public static CustomPredicate<Order> Predicate(Func<Order, bool> predicate, string error)

        {

            return new CustomPredicate<Order>(predicate, error);

        }

    }

}

Please do not validate real world / business logic of those bool expression, but just an approach to validate them.

Short answer

Yes, it's over-engineered.

Long answer

You mention that it reduces the number of if statements, but not why this is your goal. In fact, it's pretty clear that reducing the number of if statements isn't inherently a target of clean code, it's something you might want to do to achieve another goal. The possibilities that come to mind are:

Enhance readibility: Your design has added a lot of code, including a new class and a helper method, and it's separated the definition of the checks away from where they're actually performed. It's a lot harder to read than if you'd just used in-line ifs.

Improve testability: If you have a combinatorial explosion of conditions, it can be useful to be able to inject in a simple condition which always passes or always fails for your unit tests, then test the actual conditions separately. But it doesn't seem like you have that issue, or that your design intends to allow that kind of injection. Really all it means is that you have more complex code, which is now harder to fully test.

So I think this makes things worse rather than better. If you want to improve readability, there's a fairly common pattern for checking preconditions which looks like:

private void Foo(int someNumber, string someText)

{

    CheckPrecondition(someNumber > 0, "Numbers must be positive");

    CheckPrecondition(someText != null, "Text can't be null");



    // Rest of the method

}

This just requires a single helper method. No classes and no Funcs. In your case you'd also want to pass the list of errors so that the check method could add its error if there was one. Even that is additional complexity and indirection for a relatively marginal benefit, but I think it's preferable to your version.