HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Rational number implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
implementationnumberrational

Problem

I've been recently working on a custom rational number implementation. Due to not very interesting reasons, using this implementation is not an option.

I'd appreciate comments of anyone who has the time to review the code. The posted code is free to use, so you can make good use of it should you want/need to.

The implementation is basically finished except the Parse and TryParse methods which I'm still working on.

I'm particularly interested in the FromDouble and TryFromDouble methods which I think are working relatively well but any suggestions, improvements or discovery of any, so far, undetected bugs are very welcomed.

```
using System;
using System.Diagnostics;
using System.Globalization;
using System.Numerics;

namespace MathSuite.Core.Numeric
{
[Serializable]
public struct Rational : IFormattable, IEquatable, IComparable, IComparable
{
#region Static fields
const string positiveInfinityLiteral = "Infinity";
const string negativeInfinityLiteral = "-Infinity";
const string naNLiteral = "NaN";
const int fromDoubleMaxIterations = 25;

static readonly Rational one = new Rational(1);
static readonly Rational zero = new Rational(0);
static readonly Rational naN = new Rational(0, 0, true);
static readonly Rational positiveInfinity = new Rational(1, 0, true);
static readonly Rational negativeInfinity = new Rational(-1, 0, true);
#endregion

#region Instance fields
readonly BigInteger numerator, denominator;
readonly bool explicitConstructorCalled, isDefinitelyIrreducible;
#endregion

#region Constructors
[DebuggerStepThrough]
public Rational(BigInteger numerator)
: this(numerator, 1, true) { }

[DebuggerStepThrough]
public Rational(BigInteger numerator, BigInteger denominator)
: this(numerator, denominator, false) { }

[DebuggerStepThrough]
private

Solution

I noticed this because naN looked really weird to my eyes. It's almost always seen as NaN. So I looked around a bit and found this.

static readonly Rational one = new Rational(1);
   static readonly Rational zero = new Rational(0);
   static readonly Rational naN = new Rational(0, 0, true);
   static readonly Rational positiveInfinity = new Rational(1, 0, true);
   static readonly Rational negativeInfinity = new Rational(-1, 0, true);


These are all implicitly private, which explains the camelCasing, but should they be? These are the kinds of static members that usually get exposed to the outside world, particularly when creating structs. Client code is going to need these.

Never mind. After much digging through the code, I see that you are exposing them to the outside world through getters...

public static Rational PositiveInfinity { get { return positiveInfinity; } }

   public static Rational NaN { get { return naN; } }

   public static Rational NegativeInfinity { get { return negativeInfinity; } }

   public static Rational Zero { get { return zero; } }


This is overkill in my opinion. The likelihood of ever needing to change the implementation is next to nil so having properties for these doesn't make much sense. It just clutters and confuses the code. Minimally, the public getters need to be declared much much closer to the private fields.

Code Snippets

static readonly Rational one = new Rational(1);
   static readonly Rational zero = new Rational(0);
   static readonly Rational naN = new Rational(0, 0, true);
   static readonly Rational positiveInfinity = new Rational(1, 0, true);
   static readonly Rational negativeInfinity = new Rational(-1, 0, true);
public static Rational PositiveInfinity { get { return positiveInfinity; } }

   public static Rational NaN { get { return naN; } }

   public static Rational NegativeInfinity { get { return negativeInfinity; } }

   public static Rational Zero { get { return zero; } }

Context

StackExchange Code Review Q#95681, answer score: 5

Revisions (0)

No revisions yet.