Replaced the State property with the _normalizationNotApplied field

[lipchev]

.. as discussed in #51

..and added DataContract/DataMember annotations (with tests)

[danm-de]

Is there a reason why the denominator is not explicitly set to BigInteger.One?

[danm-de]

Is there a reason why the denominator is not explicitly set to BigInteger.One?

[danm-de]

Is there a reason why the denominator is not explicitly set to BigInteger.One?

[danm-de]

I don't understand why we would need this additional test. What speaks against the following?

public static Fraction Abs(Fraction fraction) =>
        new(fraction._normalizationNotApplied, BigInteger.Abs(fraction.Numerator), BigInteger.Abs(fraction.Denominator));

[danm-de]

This is important if the data type is to be transferred via marshaling (P/Invoke). However, there is no (Windows) API that could handle this data type.

[danm-de]

Is there a reason why the denominator is not explicitly set to BigInteger.One?

[danm-de]

Is there a reason why the denominator is not explicitly set to BigInteger.One?

[lipchev]

There is only one usages left of the State property from the Fractions project - i must have missed it:

private static string FormatGeneral(Fraction fraction, IFormatProvider formatProvider) {
    var numberFormatInfo = (NumberFormatInfo)formatProvider.GetFormat(typeof(NumberFormatInfo))
                           ?? CultureInfo.InvariantCulture.NumberFormat;

    if (fraction.State == FractionState.IsNormalized) {

Overall, it doesn't look pretty, having the reason with the negated field name... Technically, it doesn't really matter for the [DataMember] - the FractionState would work as well, although ideally we'd want to have it as the IsNormalized version as default so that we could benefit from the EmitDefaultValue=false

[danm-de]

Changing the default value of FractionState.IsNormalized would cause 3rd-party libraries (that were compiled against the old version 7) to no longer be binary compatible. As far as I can remember, the enum values ​​are stored as constant numbers in the binary. Please correct me if I'm wrong.

I was think of extending the FractionState enum. A manageable breaking change, as NaN and Infinity were not yet supported.

csharp

/// <summary>
/// The fraction's state.
/// </summary>
[Flags]
public enum FractionState
{
    /// <summary>
    /// Unknown state.
    /// </summary>
    Unknown = 0x00000000,

    /// <summary>
    /// A reduced/simplified fraction.
    /// </summary>
    IsNormalized = 0x00000001,

    /// <summary>
    /// The fraction is not a number
    /// </summary>
    IsNaN = 0x00000002,

    /// <summary>
    /// The fraction is infinite
    /// </summary>
    IsInfinity = 0x00000004,

    /// <summary>
    /// The fraction is positive infinity
    /// </summary>
    IsPositiveInfinity = 0x00000008,

    /// <summary>
    /// The fraction is negative infinity
    /// </summary>
    IsNegativeInfinity = 0x00000010
}

and

/// <summary>
/// The fraction's state.
/// </summary>
public FractionState State =>
    (_normalizationNotApplied ? 0x0 : FractionState.IsNormalized) |
    (IsInfinity ? FractionState.IsInfinity : 0x0) |
    (IsPositiveInfinity ? FractionState.IsPositiveInfinity : 0x0) |
    (IsNegativeInfinity ? FractionState.IsNegativeInfinity : 0x0);

[lipchev]

Changing the default value of FractionState.IsNormalized would cause 3rd-party libraries (that were compiled against the old version 7) to no longer be binary compatible. As far as I can remember, the enum values ​​are stored as constant numbers in the binary. Please correct me if I'm wrong.

I'm not sure what you mean- if we're talking about serializing the enum then I believe the answer is "it depends on the serializer". If stored as integers, then the extended enum would likely be reconstructed correctly (if extending).

If you mean that the code that was previously switch-ing on the State, then I don't think the behavior would change. However, if you have something like _ => throw .. for the default case, then that's a breaking change.
However, the bigger breaking change regarding the code is the return value of Fraction.Zero.Denominator.

[danm-de]

If you mean that the code that was previously switch-ing on the State, then I don't think the behavior would change. However, if you have something like _ => throw .. for the default case, then that's a breaking change. However, the bigger breaking change regarding the code is the return value of Fraction.Zero.Denominator.

Yes, that is a big one.

I mean this:

https://learn.microsoft.com/en-us/dotnet/core/compatibility/library-change-rules

❌ DISALLOWED: Changing the value of a public constant or enumeration member

[lipchev]

Extending the FractionState like you did could have some potential performance benefits, if stored as field (instead of having the Nullable). However I'm not sure that the combination of IsNormalized and Unknown with NaN and the others don't seem like they really belong together..

[danm-de]

Extending the FractionState like you did could have some potential performance benefits, if stored as field (instead of having the Nullable).

These are just ideas. To make the State property a bit more useful. I wouldn't want to put the NaN/Infinity status there anymore. The reason is that you have already implemented the variant with _denominator == 0 -> NaN or Infinity.

[danm-de]

I have partially adopted the pull request. I removed the DataContract attribute because I don't like the results (both JSON and XML). Strictly speaking, I didn't like the result of serializing BigInteger.

[lipchev]

I don't like the JSON version either, and I don't believe anybody should be using it over the json converter. However the xml-serialization is required for the WCF proxy generation- it's an out of the box thing, and there is no longer a way (since .net core) to create a custom converter (what used to be called a Surrogate).
There are also other tools could be using these (such as for protobuf message-contracts).
Plus the whole thing is an opt-in..

[danm-de]

I haven't used WCF in years. However, I remember that the XML/JSON serialization could be influenced somehow. Maybe one can do something for the .NET Framework using the ISerializable attribute - I can't remember 😞 .

An acceptable result (for me) would be something like this:

{
  "Numerator": "-123456789",
  "Denominator": "1",
  "NormalizationNotApplied": false
}

<Fraction ...>
  <Numerator>-123456789</Numerator>
  <Denominator>1</Denominator>
  <NormalizationNotApplied>false</NormalizationNotApplied>
</Fraction>

If a field is null or a bool value is false, you could even leave them out completely.

[lipchev]

There isn't any practical way of influencing the way the default DataContractJsonSerializer serializes its [DataContract]. The only reasonable solution is to System.Text or Newtonsoft. However that is the default for just about anybody who wants to return json data- even changing the DateTime format of the DataContractJsonSerializer is a near impossible task.

Same restrictions apply to the xml-based serialization- WCF requires the type to be a "known type" or a [DataContract].

Here is how customizing the data-contracts used to be done, but those types are only available for .NET Framework.

[danm-de]

@lipchev could please checkout #55 ?

Is WCF serialization possible again?

Unfortunately, I can't say anything about the performance loss since the data type is no longer marked as readonly.

[danm-de]

Oh man, I really hate serialization... Look at the result of the DataContractJsonSerializer ... :-(

The only way to get a "correct" JSON output, is to use the (obsolete) ISerializable interface.

using System;
using System.Globalization;
using System.Numerics;
using System.Runtime.Serialization;
using System.Security.Permissions;

namespace Fractions;

[Serializable]
public partial struct Fraction : ISerializable {
    private Fraction(SerializationInfo info, StreamingContext context) {
        if (info == null) {
            throw new ArgumentNullException(nameof(info));
        }

        var numerator = BigInteger.Parse(info.GetString(nameof(Numerator)) ?? "0", CultureInfo.InvariantCulture);
        var denominator = BigInteger.Parse(info.GetString(nameof(Denominator)) ?? "0", CultureInfo.InvariantCulture);
        var normalizationNotApplied = info.GetBoolean("NormalizationNotApplied");
        this = new Fraction(normalizationNotApplied, numerator, denominator);
    }

    [SecurityPermission(SecurityAction.Demand, SerializationFormatter = true)]
    void ISerializable.GetObjectData(SerializationInfo info, StreamingContext context) {
        if (info == null) {
            throw new ArgumentNullException(nameof(info));
        }

        info.AddValue(nameof(Numerator), Numerator.ToString(CultureInfo.InvariantCulture));
        info.AddValue(nameof(Denominator), Denominator.ToString(CultureInfo.InvariantCulture));
        info.AddValue("NormalizationNotApplied", _normalizationNotApplied);
    }
}