Removed the obsolete methods (Min/Max) from UnitMath

[lipchev]

Removed the obsolete methods from UnitMath:

• Min(IEnumerable, TUnit)
• Min(IEnumerable, Func)
• Max(IEnumerable, TUnit)
• Max(IEnumerable, Func)

[lipchev]

@angularsen We're currently left with one extension method (Abs(this TQuantity)) and 3 static functions:

• Min(TQuantity, TQuantity)
• Max(TQuantity, TQuantity)
• Clamp(TQuantity, TQuantity, TQuantity)

Here are my comments from the other thread regarding these:

• the Abs function is typically part of the INumberBase interface, where it is defined as a static function (same as IsNegative or IsFinite), so typically the expectation would be to look for a static function.
• same goes for the Max of two numbers - in the current code-base we have to use UnitMath.Max(first, second) or UnitMath.Clamp (which also happens to be part of the INumber interface).

I wouldn't mind keeping the Min/Max methods as they are - we/the users can later (net10) add a static extension function for TQuantity.Abs.
However if you want the Abs as an extension method of TQuantity, I think we should constrain it to be an ILinearQuantity (as I don't see it being applicable to the other types).

[angularsen]

However if you want the Abs as an extension method of TQuantity, I think we should constrain it to be an ILinearQuantity (as I don't see it being applicable to the other types)

I like that

[lipchev]

I was looking to remove these tests (and the functions) from the 🐲 PR and it appears that there I've actually added the linq-equivalent:

        [Fact]
        public void MinOfLengthsCalculatesCorrectlyWithLinq()
        {
            var units = new[] { Length.FromMeters(1), Length.FromCentimeters(50) };

            Length min = units.Min();

            Assert.Equal(50, min.Value);
            Assert.Equal(LengthUnit.Centimeter, min.Unit);
        }

Do you think we want such tests here?

[angularsen]

It can be useful enough to have a few of these I think, to spot mistakes when messing with comparison implementations in the future

[lipchev]

Ok here's a more subtle one- consider the following two versions of the Clamp:

        public static TQuantity Clamp<TQuantity>(TQuantity value, TQuantity min, TQuantity max)
            where TQuantity : IQuantityOfType<TQuantity>, IComparable<TQuantity>
        {
            UnitKey targetUnit = value.UnitKey;
            TQuantity minValue = UnitConverter.Default.ConvertToUnit(min, targetUnit);
            TQuantity maxValue = UnitConverter.Default.ConvertToUnit(max, targetUnit);

            if (minValue.CompareTo(maxValue) > 0)
            {
                throw new ArgumentException($"min ({min}) cannot be greater than max ({max})", nameof(min));
            }

            if (value.CompareTo(minValue) < 0)
            {
                return minValue;
            }

            if (value.CompareTo(maxValue) > 0)
            {
                return maxValue;
            }

            return value;
        }

        public static TQuantity Clamp<TQuantity>(TQuantity value, TQuantity min, TQuantity max)
            where TQuantity : IQuantity, IComparable<TQuantity>
        {
            if (min.CompareTo(max) > 0)
            {
                throw new ArgumentException($"min ({min}) cannot be greater than max ({max})", nameof(min));
            }

            if (value.CompareTo(min) < 0)
            {
                return min;
            }

            if (value.CompareTo(max) > 0)
            {
                return max;
            }

            return value;
        }

They both have one thing in common: they avoid calling the CompareTo(object) .

The difference is obviously in whether the Unit of the returned quantity is the same as the input value (this is the original behavior) and consequently the implementation thus imposes slightly different constraints on the type parameters.

The good news is that since the IComparable interface doesn't appear anywhere on the IQuantity interfaces, all these extensions are only applicable with the concrete types.

There are no bad news here, just wondering if you'd rather keep the original behavior regarding the unit or not (personally if I was writing this function from scratch I would have preferred the second version, but I guess someone must have had a reason to do it the other way..).

[angularsen]

I'm not entirely sure, these two seem very equivalent to me, but off the bat I like the second version better with fewer constraints.

[angularsen]

LGTM merging

Remaining points:

• Abs as extension method Removed the obsolete methods (Min/Max) from UnitMath #1601 (comment)
• Remove one of the two Clamp methods Removed the obsolete methods (Min/Max) from UnitMath #1601 (comment)
• Tests to keep in fractions PR Removed the obsolete methods (Min/Max) from UnitMath #1601 (comment)