Wrapper class for min/max clamping and validation

Question

I have a lot of classes that have numeric values that are configurable by the customer during runtime and should be clamped between a min and a maximum value. The value changes should also be logged. An example of this is given below:

public class MyClass 
{
    public double AMin { get; set; }
    public double AMax { get; set; }
    public double A { get; private set; }
    public double BMin { get; set; }
    public double BMax { get; set; }
    public double B { get; private set; }
    public double CMin { get; set; }
    public double CMax { get; set; }
    public double C { get; private set; }

    public bool SetA(double value) 
    {
        double clamped = Clamp(value, AMin, AMax);
        if (clamped != value) 
        {
            LogError("New value: {value} for A is out of range.");
            return false;
        }
        else 
        {
            LogError("Set A to: {value}.");
            return true;
        }
    }

    public bool SetB(double value) 
    {
        double clamped = Clamp(value, BMin, BMax);
        if (clamped != value) 
        {
            LogError("New value: {value} for B is out of range.");
            return false;
        }
        else 
        {
            LogError("Set B to: {value}.");
            return true;
        }
    }

    public bool SetC(double value) 
    {
        double clamped = Clamp(value, CMin, CMax);
        if (clamped != value) 
        {
            LogError("New value: {value} for C is out of range.");
            return false;
        }
        else 
        {
            LogError("Set C to: {value}.");
            return true;
        }
    }

    public static double Clamp(double value, double min, double max) 
    {
        if (value <= min) 
        {
            _value = min;
            return;
        }

        if (value >= max) 
        {
            _value = max;
            return;
        }

        _value = value;
    }
}

As the number of values grow, you can see this has me writing boilerplate code and it's easy to make copy/paste errors. I could make a Visual Studio snippet for this, but I was wondering if it would be a good solution if I would refactor this repetition into a class, so that it would look like below:

public class MyClass 
{
    public Parameter A { get; init; } = new Parameter("A");
    public Parameter B { get; init; } = new Parameter("B");
    public Parameter C { get; init; } = new Parameter("C"); 
    ...
}

public class Parameter 
{
    public string Name { get; set; } = "";

    public double Min { get; set; } = double.MinValue;
    public double Max { get; set; } = double.MaxValue;

    public double Value { get => _value; }

    private double _value = 0.0;

    public Parameter(string name) 
    {
        Name = name;
    }

    public double GetValue() 
    {
        return _value;
    }

    // returns true on success
    public bool SetValue(double value) 
    {
        if (value <= Min) 
        {
            _value = Min;
            Log.Error($"New value: {value} for {Name} too low.");
            return false;
        }

        if (value >= Max) 
        {
            _value = Max;
            Log.Error($"New value: {value} for {Name} too high.");
            return false;
        }

        Log.Info($"Set {Name} to: {value}.");

        _value = value;
    }    
}

Although this would have me writing MyInstance.MyParameter.Value instead of MyInstance.MyParameter all the time, and it "violates" the law of Demeter, I think in the end it will save me a lot of time and I think it would be more maintainable.

Is it a common practice to do something like this, or is there a big downside that I am not yet seeing?

Side-note: I work in industrial automation, more specific with PLCs, so validation using exceptions is not an option for me. I have a C# background, hence the example is given in that language.

Answer 1

Reusability

As the number of values grow, you can see this has me writing boilerplate code and it's easy to make copy/paste errors. I could make a Visual Studio snippet for this, but I was wondering if it would be a good solution if I would refactor this repetition into a class,

100% the right call to make. Repetition should be converted into reusability.

---

Law of Demeter?

Although this would have me writing MyInstance.MyParameter.Value instead of MyInstance.MyParameter all the time, and it "violates" the law of Demeter

Just to be clear, because I suspect this lies at the basis of your conclusion, the law of Demeter is not a dot counting exercise.

The issue here is that if you do the classic property-wrapping and hide the extra layer, that will also hide the boolean return value that you're working with. Assuming this is a needed feature (because otherwise why have it?), you wouldn't want to hide that from the consumer who attempts to set the value.

MyInstance.MyParameter.SetValue() makes the most sense here specifically because it reveals the clamped nature of the value and allows the consumer to retrieve that boolean value to indicate if clamping occurred or not.

However, you could change the getter logic to use an implicit conversion:

public static implicit operator double(Parameter p)  
{  
   return p.Value;  
}

This would enable you to do things like:

var myInstance = new MyClass();
bool wasClamped = myInstance.A.SetValue(1.23);
double myValue = myInstance.A;

Note that in the third line you can now skip the .Value step due to the implicit conversion, but you can't avoid the .SetValue() call in the second line as you need it to receive the boolean value.

Whether you implement that implicit operator or instead just accept having to use .Value, both are justifiable solutions. Pick what you prefer.

---

Some short form code review

This is not the focus of your question but it seems useful to mention anyway:

public double GetValue() 
{
    return _value;
}

This method can be removed since Value serves the same purpose.

public double Value { get => _value; }

private double _value = 0.0;

This can be condensed into public double Value { get; private set }. You don't need to set it to 0.0 explicitly since that is the default value anyway.

public Parameter A { get; init; } = new Parameter("A");

It might be nicer to use nameof(A) in the constructor to keep the code refactor-friendly.

More importantly, I suspect you don't want the init here, because do you really want the consumer to be able to override the parameter instance?

Additionally, I find it weird that you make your min/max boundaries publicly settable. I instinctively would've expected them to be forced in the constructor and then kept immutable, specifically so your parent MyClass can configure the parameter and keep it that way and the consumers of MyClass are not able to change that configuration.

This also factors into the default value: what if the default 0.0 is not in the defined range? This is a concrete concern for your current code, as I can set a value and then change the boundaries. I don't think that that is desirable behavior.

I would also be inclined to allow the constructor to optionally pass the initial value.

One more thing: this Parameter class can very clearly benefit from being generic, as a way to maximize reusability.

Taking all of this into account, I would be more inclined to approach it immutably:

public class MyClass 
{
    // Initial value unspecified, will try to be 0 but within clamping rules
    public Parameter<double> A { get; } = new Parameter<double>(nameof(A), 1, 10);

    // Initial value explicitly defined, but will still be clamped!
    public Parameter<double> B { get; } = new Parameter<double>(nameof(B), 1, 10, 5);
}

public class Parameter<T>
{
    public string Name { get; }
    public T Min { get; }
    public T Max { get; }

    public Parameter(string name, T min, T max, T initialValue)
    {
        this.Name = name;
        this.Min = min;
        this.Max = max;

        this.SetValue(initialValue);
    }

    public Parameter(string name, T min, T max)
      : this(name, min, max, default) 
    {}

    // ...
}