asulwer / RulesEngine

Rules Engine with extensive Dynamic expression support
MIT License
6 stars 1 forks source link

remove params from IRulesEngine and inherited classes #26

Closed asulwer closed 5 days ago

asulwer commented 6 days ago

replace use of params with proper array

RenanCarlosPereira commented 5 days ago

we won't be able to remove it, as we will introduce breaking changes

asulwer commented 5 days ago

its considered bad practice to use params. i have not removed it but in the future i think we should. parameter defaults and params, which goes last? params and its ugly. this is my suggestion. we leave it until people migrate over, old code stays. but anything new, we dont use params, find another way.

we are not taking full/proper advantage of System.Linq.Dynamic.Core which this library, basically, wraps. i have some examples that i have been working with for the use of lambda expressions. this would remove the, basically, hardcoded parameter names

this line is the start of what i am referring to.

public async ValueTask<List<RuleResultTree>> ExecuteAllRulesAsync(string workflowName, object[] inputs, CancellationToken cancellationToken)
{
    var ruleParams = new List<RuleParameter>();

    for (var i = 0; i < inputs.Length; i++)
    {
        if (cancellationToken.IsCancellationRequested)
            break;

        var input = inputs[i];
        ruleParams.Add(new RuleParameter($"input{i + 1}", input));
    }

    return await ExecuteAllRulesAsync(workflowName, ruleParams.ToArray());
}

instead of defining what the name of the parameter is i can be inferred by the type used in the expression.

here is a minor example of the expressions i want to move towards

public static class Utils
{
    public static IEnumerable<T> SetValue<T>(this IEnumerable<T> items, List<Action<T>> updateMethods)
    {
        foreach (var item in items)
        {
            foreach(var method in updateMethods)
                method(item);
        }
        return items;
    }
}
public class Card : IEquatable<Card>
{
    public string Name { get; set; } = string.Empty;

    public bool Equals(Card? other) => Name == other?.Name;
    public override bool Equals(object? obj) => Equals(obj as Card);
    public override int GetHashCode() => Name.GetHashCode();
}

var cards1 = new List<Card> { new Card { Name = "Fart - FOIL" }, new Card { Name = "Fart - Foil" } };
var cards2 = new List<Card> { new Card { Name = "Fart (Foil)" } };

var newCards1 = cards1.SetValue([
    c => c.Name = c.Name.Replace(" - Foil", ""),
    c => c.Name = c.Name.Replace(" - FOIL", "")])
    .Union(cards2.SetValue(c => c.Name = c.Name.Replace(" (Foil)", "")))
    .ToList();

c in the expression is of type Card. the way RulesEngine injects, in my example, Card type into that expression is not ideal and prone to runtime errors.

RenanCarlosPereira commented 5 days ago
  1. Using Params:

    Params isn’t inherently bad practice; it makes method calls more flexible and concise. The downside is it can lead to runtime errors if misused. As for the order, params must be the last parameter, which is just how it works. While it might seem "ugly" to some, it’s often handy.

    • Why Params Isn't Bad Practice:

      Flexibility: params allows methods to accept a variable number of arguments, making the method more versatile. It simplifies method calls by allowing a variable number of parameters without needing to explicitly define multiple overloads (source).

      Conciseness: Using params can make code cleaner by avoiding overloaded methods or collections as parameters. It reduces the boilerplate code and enhances readability (source).

      Readability: Properly documented params can make the method signature clearer and easier to understand. Discussions on platforms like Stack Overflow show various use cases where params improve code quality and maintainability (source).

  2. Parameter Naming with Property Selection:

    Instead of defining the parameter name as $"input{index + 1}", you can use a lambda expression to select the property name. I would suggest to open another issue, there's many ways of doing that.

asulwer commented 4 days ago

params is like using goto, both have their place but alternatives exist.

  1. A bloat of possible run type errors
  2. A lot of unnecessary casts.
  3. Difficulty understanding what a method does by its signature.
  4. A lack of contracts of the interface of a class. If all parameters are of Object type then you aren't declaring anything informative to the client classes.
  5. A lack of overloading possibilities.
  6. The incorrectness of override. You are able to override a method and change its parameter types thus breaking everything which is inheritance related.