5
\$\begingroup\$

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.

edit2

For simplicity let's just use those models

public class Statuses
{
    public const string 
        Status_Creating = "Order is during creation process.",
        Status_SentNotConfirmed = "Order is sent to company, but not confirmed yet.",
        Status_Accepted = "Order got accepted by company.",
        Status_Rejected = "Order got accepted by company.",
        Status_InProgress = "Order is in progress.";
}

public class Company
{
    public Guid Id { get; } = Guid.NewGuid();
}

public class Order
{
    public Guid Id { get; } = Guid.NewGuid();
    public Company Company { get; set; }

    public List<Product> Products { get; set; } = new List<Product>();

    public string ContactInfo { get; set; }
}

public class Product
{
    public Guid Id { get; } = Guid.NewGuid();

    public string CustomerNote { get; set; }
    public string SomeProperty { get; set; }
    public string SomeProperty2 { get; set; }

    public bool HasArrived()
    {
        return DateTime.Now >= Arrival;
    }

    public DateTime Arrival { get; set; }
}
\$\endgroup\$
  • \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How to Ask for examples, and revise the title accordingly. \$\endgroup\$ – Zeta Feb 13 at 10:40
  • \$\begingroup\$ Can you post other types too so that the code compiles without errors? \$\endgroup\$ – t3chb0t Feb 13 at 15:29
  • 1
    \$\begingroup\$ Are you open to using 3rd-party libraries? What you're building looks a lot like FluentValidation to me--just implement an AbstractValidator<Order> for each state transition. \$\endgroup\$ – xander Feb 14 at 1:49
  • \$\begingroup\$ @t3chb0t Try now \$\endgroup\$ – Joelty Feb 14 at 7:56
  • \$\begingroup\$ @xander Sounds like a good idea \$\endgroup\$ – Joelty Feb 14 at 8:01
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ It's a lot harder to read than if you'd just used in-line ifs. I agree, but I thought about it as: Ok, so I have e.g 8 different statuses and every of them has at least 5 checks, so I have switch with 8 cases + 8 methods which contain 5+ ifs - it's a lot CheckPrecondition what are overloads of this method? No classes and no Funcs What's wrong with them? \$\endgroup\$ – Joelty Feb 13 at 13:22

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.