Question: In Fraction constructor, why do you convert BigInteger to byte array and back again?

[stewienj]

In fraction.cs you have this constructor:

public Fraction(BigInteger numerator, BigInteger denominator)
{
    Numerator = new BigInteger(numerator.ToByteArray());
    Denominator = new BigInteger(denominator.ToByteArray());
}

I was comparing your implementation with the old Microsoft BigRational, and for my usage pattern the old (but buggy) Microsoft one was up to 10x faster, and the reason was the above constructor.

BigInteger is a struct, so it will be copied by value. If there's an issue that can be fixed by the above code then there is a big problem at play somewhere. I changed the code to the following and it still passes all your unit tests and at the same time speeds up my code 10 fold.

public Fraction(BigInteger numerator, BigInteger denominator)
{
    Numerator = numerator;
    Denominator = denominator;
}

Could you please enlighten me about this issue. Here's some code that I was using in my comparison test:

private readonly int _iterations = 1_000;
private readonly int _count = 1_000;

[TestMethod]
public void TestIteratingOnInsertionIndexDownDirectionAlternate2()
{
    List<ExtendedNumerics.BigRational> testSetLower = new List<ExtendedNumerics.BigRational>(_count);
    List<ExtendedNumerics.BigRational> testSetUpper = new List<ExtendedNumerics.BigRational>(_count);
    for (int i = 0; i < _count; ++i)
    {
        testSetLower.Add((ExtendedNumerics.BigRational)i);
        testSetUpper.Add((ExtendedNumerics.BigRational)(i + 1));
    }
    for (int iteration = 0; iteration < _iterations; ++iteration)
    {
        for (int i = 0; i < _count; ++i)
        {
            // Get pairs of number, take half way between them, and check it
            // is less than the high end and greater than the lower end
            ExtendedNumerics.BigRational newValue = (testSetLower[i] + testSetUpper[i]) / (ExtendedNumerics.BigRational)2;
            Assert.IsTrue(newValue > testSetLower[i]);
            Assert.IsTrue(newValue < testSetUpper[i]);
            testSetUpper[i] = newValue;
        }
    }
}

[stewienj]

I notice in the newest version of BigInteger the whole struct has been marked readonly, which is nice. https://github.com/dotnet/runtime/blob/master/src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs I decompiled the version from .NET 4.6.1. It appears that only the constructors modify the internal array member, called _bits. I carefully inspected some areas of code, like the bit shift operators, and copies of the bits are made before manipulation is done. Note the area at the bottom of this code block:

public static BigInteger operator >>(BigInteger value, int shift) {
            if (shift == 0) return value;
            else if (shift == Int32.MinValue) return ((value << Int32.MaxValue) << 1);
            else if (shift < 0) return value << -shift;

            int digitShift = shift / kcbitUint;
            int smallShift = shift - (digitShift * kcbitUint);

            uint[] xd; int xl; bool negx;
            negx = GetPartsForBitManipulation(ref value, out xd, out xl);

            if (negx) {
                if (shift >= (kcbitUint * xl)) {
                    return BigInteger.MinusOne;
                }
                uint[] temp = new uint[xl];
                Array.Copy(xd /* sourceArray */, temp /* destinationArray */, xl /* length */);  // make a copy of immutable value._bits
                xd = temp;
                NumericsHelpers.DangerousMakeTwosComplement(xd); // mutates xd
            }

[AdamWhiteHat]

Yeah, the idea was to clone the object.

I'm not sure why I thought that was necessary; its been many years since I wrote it. There may not have been a good reason for it, and I was just being a paranoid parrot. I was certainly guilty of programming by coincidence on occasion during this period in my relationship with C#. I do remember getting pretty desperate trying to get consistent behavior when converting and casting to and from the various types. This may just be a relic left over from that.

We can probably just strip this out of here; you are right that it should not matter and it does have a noticeable impact on performance, if you're calling the constructor a million times.

[AdamWhiteHat]

https://www.nuget.org/packages/ExtendedNumerics.BigRational/1.1.0.50000