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

My Rational struct, version 1

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

Problem

I have written a Rational struct for working with rational numbers, i.e. Rational(int numerator, int denominator).

A recent post regarding rational numbers peaked my interest. I particular like the answer by @aush with his RationalNumber class. I am inclined to think of a rational number as just that: a number, akin to { int, double, Decimal }. So it screams for struct rather than a class.

Note I am neither a student, nor a teacher. Though I write code for a living, I have no business requirements for Rational, which means I have no constraints or restrictions on the struct design. I am only limited by what I can imagine how a rational number should behave. In fact, I have no foreseen practical need for Rational. I just find that going through such mental exercises helps improve my overall skills.

```
namespace System
{
public struct Rational : IComparable, IComparable, IEquatable
{
public int Numerator { get; private set; }
public int Denominator { get; private set; }

// These fields bypass Simplify().
public static readonly Rational MinValue = new Rational { Numerator = int.MinValue, Denominator = 1 };
public static readonly Rational MaxValue = new Rational { Numerator = int.MaxValue, Denominator = 1 };
public static readonly Rational Epsilon = new Rational { Numerator = 1, Denominator = int.MaxValue };
public static readonly Rational Undefined = new Rational { Numerator = 0, Denominator = 0 };
public static readonly Rational Zero = new Rational { Numerator = 0, Denominator = 1 };
public static readonly Rational One = new Rational { Numerator = 1, Denominator = 1 };
public static readonly Rational MinusOne = new Rational { Numerator = -1, Denominator = 1 };

public Rational(int numerator, int denominator = 1) : this()
{
this.Numerator = numerator;
this.Denominator = denominator;
// There is a speci

Solution

I like it very much (maybe other reviewers will disagree though).

A few things tickle a bit.
System namespace is not yours

I wouldn't put anything in the System namespace, whatever the reason is. That namespace belongs to the framework, and as much as your struct looks like it should be part of that framework, it isn't. And the day Microsoft ships a System.Rational, you have a clash.
this

The keyword this is inconsistently being used as a qualifier - sometimes it's there, sometimes it isn't. I'd just remove it, it's perfectly redundant.
ToString

The ToString implementation is switching on a non-enum value, and that switch block doesn't have a default case:

public override string ToString()
    {
        switch (Denominator)
        {
            case 0:
                return "Undefined";
            case 1:
                return Numerator.ToString();
        }
        return string.Format("{0}/{1}", Numerator, Denominator);
    }


I'd live with the non-enum switch in the name of premature optimisation (does an if block really make a difference?), but move the return string.Format part into it.
Double-dip

In several places you're doing this:

return new Rational
{
    Numerator = /*expression*/,
    Denominator = /*expression*/
}.Simplify();


That's actually calling the default constructor (a 0/0 / Undefined instance) and accessing the private setters from outside that instance, which is violating the type's immutability and, some would argue, encapsulation. And then Simplify() returns yet another instance.

I don't like having public int SomeProperty { get; private set; } auto-properties in an immutable type, exactly for that reason. In my mind, a public property with a private setter should be 100% equivalent to this:

private readonly int _numerator;
public int Numerator { get { return _numerator; } }


But that would blow up your code with the above, because you're accessing the private setter.

Why not just do this instead?

return new Rational([expression], [expression]);


The constructor is calling Simplify anyway!
Immutable?

Wait a sec... is that struct really immutable?

private void Reduce()
{
    var greatestCommonDivisor = GreatestCommonDivisor(Numerator, Denominator);
    Numerator /= greatestCommonDivisor;
    Denominator /= greatestCommonDivisor;
}


Shouldn't that be private Rational Reduce(), and returning a new instance? You're dealing with a struct here - the two values ought to be considered as a single unit. I'm not sure how much of a violation that is, because the method is private, but it doesn't feel right that a struct internally mutates itself.

Code Snippets

public override string ToString()
    {
        switch (Denominator)
        {
            case 0:
                return "Undefined";
            case 1:
                return Numerator.ToString();
        }
        return string.Format("{0}/{1}", Numerator, Denominator);
    }
return new Rational
{
    Numerator = /*expression*/,
    Denominator = /*expression*/
}.Simplify();
private readonly int _numerator;
public int Numerator { get { return _numerator; } }
return new Rational([expression], [expression]);
private void Reduce()
{
    var greatestCommonDivisor = GreatestCommonDivisor(Numerator, Denominator);
    Numerator /= greatestCommonDivisor;
    Denominator /= greatestCommonDivisor;
}

Context

StackExchange Code Review Q#84365, answer score: 6

Revisions (0)

No revisions yet.