Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NumberExtensions should not use Convert for net8.0 #1483

Open
lipchev opened this issue Dec 30, 2024 · 4 comments
Open

NumberExtensions should not use Convert for net8.0 #1483

lipchev opened this issue Dec 30, 2024 · 4 comments

Comments

@lipchev
Copy link
Collaborator

lipchev commented Dec 30, 2024

I just noticed this simple performance improvement that's likely to have a huge performance impact on the NumberExtensions..

        public static Mass Centigrams<T>(this T value)
            where T : notnull
#if NET7_0_OR_GREATER
            , INumber<T>
            => Mass.FromCentigrams(double.CreateChecked(value));
#else
            => Mass.FromCentigrams(Convert.ToDouble(value));
#endif

Not sure if we want this in v5, v6 or I could just put it up-for-grabs and leave it for later..

Note that there is probably a Convert.ToDecimal somewhere, just waiting to popup as a merge conflict...

@lipchev
Copy link
Collaborator Author

lipchev commented Dec 30, 2024

@angularsen Oh you're gonna love this: thanks to the IConvertible interface, on net48- the following syntax should work just fine: Mass newMass = Mass.FromGrams(2).Centigrams() 😆 (of course this would be wrong)

@angularsen
Copy link
Owner

Let's not bother with v5, it can receive new units and bug fixes, but new features can mostly just go into v6 so we can focus on finalizing and shipping it before it becomes an infinite project. Scope is definitely creeping.

As for net48, I can sympathize being stuck on a legacy project, but come on! Get on with the times people! .NET Core 2.x has been available for 6 years already 😅

Good example though, something to consider when discussing IConvertible.

My current thinking is, if the argument is WPF and XAML-like bindings, then value converters are a better approach than requiring all quantities to implement IConvertible.

public class FooTests
{
#if NET48
        [Fact]
        public void Foo()
        {
            Mass newMass = NumberToMassExtensions.Kilograms(Mass.FromGrams(2000));
            Assert.Equal(Mass.FromKilograms(2), newMass);
            /*
            Assert.Equal() Failure: Values differ
            Expected: 2 kg
            Actual:   2000 kg
            */
        }
#endif

}

@angularsen
Copy link
Owner

Regarding the issue description, sounds good to me 👍

@Muximize
Copy link
Contributor

Muximize commented Jan 8, 2025

FYI, getting rid of IConvertible would clean up 99 lines of code in each of 126 quantities for a total of 12.474 lines removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants