Skip to main content
added 2255 characters in body
Source Link
Flater
  • 59.5k
  • 8
  • 112
  • 171

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.

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

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.

added 2255 characters in body
Source Link
Flater
  • 59.5k
  • 8
  • 112
  • 171

MyInstance.MyParameter.ValueSetValue() 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 you can now skip the .Value step due to the implicit conversion, but you can't avoid the .SetValue() call 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.

MyInstance.MyParameter.Value 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.

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 you can now skip the .Value step due to the implicit conversion, but you can't avoid the .SetValue() call 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.

added 2255 characters in body
Source Link
Flater
  • 59.5k
  • 8
  • 112
  • 171

Reusability


Law of Demeter?

Some short form code review

As an aside, your code lacks any meaningful configuration ofThis is not the min/maxboundaries. Settingfocus of your question but it seems useful to mention anyway:

public double GetValue() 
{
    return _value;
}

This method can be removed since MinValueValue andserves the same purpose.

public double Value { get => _value; }

private double _value = 0.0;

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

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

It might be nicer to use by definition could not exist outside of that rangenameof(A) in the constructor to keep the code refactor-friendly. I'm assuming

More importantly, I suspect you omitteddon'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 rangeboundaries 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 from.

This also factors into the example snippet because it'sdefault value: what if the default 0.0 is not in the primary focusdefined 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) 
    {}

    // ...
}

As an aside, your code lacks any meaningful configuration of the min/maxboundaries. Setting it to MinValue and MaxValue is meaningless since any value by definition could not exist outside of that range. I'm assuming you omitted the min/max range configuration from the example snippet because it's not the primary focus.

Reusability


Law of Demeter?

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) 
    {}

    // ...
}
Source Link
Flater
  • 59.5k
  • 8
  • 112
  • 171
Loading