Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Following up on this post, and including some major changes suggested, here's the revised code.

Changes include:

  • No longer keeping an IFormatProvider at instance level.
  • Removed IFormatProvider constructor parameters.
  • Introduced ToString(IFormatProvider) overload.
  • Changed decimal to float, to leverage float.NaN for 0-denominator fractions.
  • Implemented more operators.
  • Reimplemented Equals and CompareTo per recommendations.
  • Fixed a bug in ToString(string, IFormatProvider).
  • Added XML comments on all public members.

As the code file grew, it became apparent that I was going to need a way of grouping code sections. I did not want to use #region because... it's a question of principles. It's irrational, I just don't want to use #region.

So I regrouped all static members into a partial struct (note: some code lines and XML comments were reformatted to avoid horizontal scrolling):

/// <summary>
/// A fractional representation of a rational number.
/// </summary>
public partial struct Fraction
{
    /// <summary>
    /// An empty <c>Fraction</c> (0/0).
    /// </summary>
    public static readonly Fraction Empty = new Fraction();

    /// <summary>
    /// A <c>Fraction</c> representation of the integer value 0.
    /// </summary>
    public static readonly Fraction Zero = new Fraction(default(int));

    /// <summary>
    /// A <c>Fraction</c> representation of the integer value 1.
    /// </summary>
    public static readonly Fraction One = new Fraction(1);

    /// <summary>
    /// Represents the smallest possible value for a <see cref="Fraction"/>.
    /// </summary>
    public static readonly Fraction MinValue = new Fraction(1, int.MinValue);

    /// <summary>
    /// Represents the largest possible value for a <see cref="Fraction"/>.
    /// </summary>
    public static readonly Fraction MaxValue = new Fraction(int.MaxValue, 1);

    /// <summary>
    /// Returns a simplified/reduced representation of a fraction.
    /// </summary>
    /// <param name="fraction">The fraction to simplify.</param>
    /// <returns>
    /// Returns a new <see cref="Fraction"/>,
    /// a simplified representation of this instance (if simplification is possible).
    /// </returns>
    public static Fraction Simplify(Fraction fraction)
    {
        if (fraction.IsUndefined)
        {
            return new Fraction(fraction);
        }

        var gcd= GetGreatestCommonDenominator(fraction.Numerator, fraction.Denominator);

        var numerator = fraction.Numerator / gcd;
        var denominator = fraction.Denominator / gcd;

        return new Fraction(numerator, denominator);
    }

    private static int GetGreatestCommonDenominator(int numerator, int denominator)
    {
        return denominator == 0 
                   ? numerator
                   : GetGreatestCommonDenominator(denominator, numerator % denominator);
    }

    private static readonly Regex _parserRegex = 
        new Regex(@"^\s*?(?<numerator>\d+)\s*?/\s*?(?<denominator>\d+)\s*?$");

    /// <summary>
    /// Converts the string representation of a fraction 
    /// into its <c>Fraction</c> equivalent.
    /// A return value indicates whether the conversion succeeded.
    /// </summary>
    /// <param name="s">A string containing the fraction to convert.</param>
    /// <param name="result">
    /// When this method returns, contains the <c>Fraction</c> 
    /// equivalent to the specified string.
    /// </param>
    /// <returns>Returns <c>true</c> if conversion is successful.</returns>
    public static bool TryParse(string s, out Fraction result)
    {
        var syntaxMatch = _parserRegex.Match(s);
        if (!syntaxMatch.Success)
        {
            result = Fraction.Zero;
            return false;
        }

        var numerator = int.Parse(syntaxMatch.Groups["numerator"].Value);
        var denominator = int.Parse(syntaxMatch.Groups["denominator"].Value);

        result = new Fraction(numerator, denominator);
        if (!result.IsUndefined)
        {
            result = result.Simplify();
        }

        return true;
    }

    public static explicit operator float(Fraction fraction)
    {
        return fraction.ToFloat();
    }

    public static bool operator ==(Fraction fraction1, Fraction fraction2)
    {
        return fraction1.Equals(fraction2);
    }

    public static bool operator ==(Fraction fraction, int value)
    {
        return fraction.Equals(new Fraction(value));
    }

    public static bool operator ==(Fraction fraction, float value)
    {
        Fraction result;
        if (Fraction.TryParse(value.ToString(), out result))
        {
            return fraction.Equals(result);
        }

        return false;
    }

    public static bool operator !=(Fraction fraction1, Fraction fraction2)
    {
        return !(fraction1 == fraction2);
    }

    public static bool operator !=(Fraction fraction, int value)
    {
        return !(fraction == value);
    }

    public static bool operator !=(Fraction fraction, float value)
    {
        return !(fraction == value);
    }

    public static Fraction operator ++(Fraction fraction)
    {
        return new Fraction(fraction.Numerator + 1, fraction.Denominator);
    }

    public static Fraction operator +(Fraction fraction, int value)
    {
        return fraction + new Fraction(value);
    }

    public static Fraction operator +(Fraction fraction1, Fraction fraction2)
    {
        int numerator = (fraction1.Numerator * fraction2.Denominator)
                      + (fraction1.Denominator * fraction2.Numerator);
        int denominator = (fraction1.Denominator * fraction2.Denominator);

        var result = new Fraction(numerator, denominator).Simplify();
        return result;
    }

    public static Fraction operator --(Fraction fraction)
    {
        return new Fraction(fraction.Numerator - 1, fraction.Denominator);
    }

    public static Fraction operator -(Fraction fraction, int integer)
    {
        return fraction - new Fraction(integer);
    }

    public static Fraction operator -(Fraction fraction1, Fraction fraction2)
    {
        var subtrator = new Fraction(fraction2.Numerator * -1, fraction2.Denominator);
        return fraction1 + subtrator;
    }

    public static Fraction operator /(Fraction fraction, int integer)
    {
        return fraction / new Fraction(integer);
    }

    public static Fraction operator /(Fraction fraction1, Fraction fraction2)
    {
        var divisor = new Fraction(fraction2.Denominator, fraction2.Numerator);
        return fraction1 * divisor;
    }

    public static Fraction operator *(Fraction fraction, int integer)
    {
        return fraction * new Fraction(integer);
    }

    public static Fraction operator *(Fraction fraction1, Fraction fraction2)
    {
        var numerator = fraction1.Numerator * fraction2.Numerator;
        var denominator = fraction1.Denominator * fraction2.Denominator;

        var result = new Fraction(numerator, denominator).Simplify();
        return result;
    }
}

That left all instance members in their own file:

/// <summary>
/// A fractional representation of a rational number.
/// </summary>
[Serializable]
public partial struct Fraction : IFormattable,
                                 IComparable, 
                                 IComparable<Fraction>,
                                 IEquatable<Fraction>
{
    private readonly int _numerator;
    private readonly int _denominator;

    /// <summary>
    /// Copy constructor. 
    /// Creates a new <c>Fraction</c> instance based on the specified value.
    /// </summary>
    /// <param name="fraction"></param>
    public Fraction(Fraction fraction)
        : this(fraction.Numerator, fraction.Denominator)
    {
    }

    /// <summary>
    /// Creates a new <c>Fraction</c> with the denominator being 1.
    /// </summary>
    /// <param name="numerator"></param>
    public Fraction(int numerator)
        : this(numerator, 1)
    {
    }

    /// <summary>
    /// Creates a new <c>Fraction</c> with specified numerator and denominator.
    /// </summary>
    /// <param name="numerator"></param>
    /// <param name="denominator"></param>
    public Fraction(int numerator, int denominator)
    {
        _numerator = numerator;
        _denominator = denominator;
    }

    /// <summary>
    /// Gets the numerator (get-only).
    /// </summary>
    public int Numerator { get { return _numerator; } }

    /// <summary>
    /// Gets the denominator (get-only).
    /// </summary>
    public int Denominator { get { return _denominator; } }

    /// <summary>
    /// Gets a value indicating whether this instance is defined.
    /// Returns true when the fraction is a division by zero.
    /// </summary>
    public bool IsUndefined { get { return _denominator == default(int); } }

    /// <summary>
    /// Simplifies/reduces the fraction.
    /// </summary>
    /// <returns>
    /// Returns a simplified representation of this instance, 
    /// if simplification is possible.
    /// </returns>
    public Fraction Simplify()
    {
        return Fraction.Simplify(this);
    }

    /// <summary>
    /// Creates a <c>float</c> representation of the <see cref="Fraction"/>.
    /// </summary>
    /// <returns>
    /// Returns the result of dividing the <c>Numerator</c> by the <c>Denominator</c>, 
    /// or <c>float.NaN</c> when the <c>Denominator</c> is zero.
    /// </returns>
    public float ToFloat()
    {
        return IsUndefined ? float.NaN 
                           : (float)_numerator / (float)_denominator;
    }

    /// <summary>
    /// Returns a value indicating whether this instance and a specified object 
    /// represent the same value.
    /// </summary>
    /// <param name="obj">Any <c>Fraction</c> or <c>float</c>-convertible value.</param>
    /// <returns></returns>
    public override bool Equals(object obj)
    {
        if (obj is Fraction)
        {
            return Equals((Fraction)obj);
        }

        return ToFloat().Equals((float)obj);
    } 

    /// <summary>
    /// Returns the hash code for this instance.
    /// </summary>
    /// <returns></returns>
    public override int GetHashCode()
    {
        return ToFloat().GetHashCode();
    }

    /// <summary>
    /// Converts this fraction into a string representation, 
    /// using a default <see cref="FractionFormatter"/>.
    /// </summary>
    /// <returns></returns>
    public override string ToString()
    {
        return ToString(FractionFormatter.Default);
    }

    /// <summary>
    /// Converts this fraction into a string representation,
    /// using specified <c>IFormatProvider</c>.
    /// </summary>
    /// <param name="provider"></param>
    /// <returns></returns>
    public string ToString(IFormatProvider provider)
    {
        return ToString(null, provider);
    }

    /// <summary>
    /// Converts this fraction into a string representation,
    /// using specified <c>format</c> and <c>IFormatProvider</c>.
    /// </summary>
    /// <param name="format"></param>
    /// <param name="provider"></param>
    /// <returns></returns>
    public string ToString(string format, IFormatProvider provider)
    {
        if (provider is ICustomFormatter)
        {
            return ((ICustomFormatter)provider).Format(format, this, provider);
        }

        return FractionFormatter.Default.Format(format, this, FractionFormatter.Default);
    }

    /// <summary>
    /// Compares this instance to a specified object and 
    /// returns an indication of their relative values.
    /// </summary>
    /// <param name="obj"></param>
    /// <returns></returns>
    public int CompareTo(object obj)
    {
        if (obj is int)
        {
            return CompareTo(new Fraction((int)obj));
        }
        else if (obj is string)
        {
            Fraction fraction;
            if (Fraction.TryParse(obj as string, out fraction))
            {
                return CompareTo(fraction);
            }
        }

        return CompareTo((Fraction)obj);
    }

    /// <summary>
    /// Compares this instance to specified <c>Fraction</c> and
    /// returns an indication of their relative values.
    /// </summary>
    /// <param name="other"></param>
    /// <returns></returns>
    public int CompareTo(Fraction other)
    {
        if (IsUndefined || other.IsUndefined)
        {
            // let the framework handle NaN comparisons
            return ToFloat().CompareTo(other.ToFloat());
        }

        long left = _numerator * other.Denominator;
        long right = _denominator * other.Numerator;

        return left.CompareTo(right);
    }

    /// <summary>
    /// Returns a value indicating whether 
    /// this instance is equal to a specified <c>Fraction</c> value.
    /// </summary>
    /// <param name="other"></param>
    /// <returns></returns>
    public bool Equals(Fraction other)
    {
        return CompareTo(other) == 0;
    }
}

Here's a screenshot of a Solution Explorer view showing all members and overloads:

Fraction type members

Is everything consistent? What could be improved? @mjolka suggested to write GetGreatestCommonDenominator iteratively - I've found this code online and I find the recursive method is easier to read:

static int GetGreatestCommonDenominator(int numerator, int denominator)
{
    int remainder;

    while (denominator != 0)
    {
        remainder = numerator % denominator;
        numerator = denominator;
        denominator = remainder;
    }

    return numerator;
}

Am I sacrificing something here?


The FractionFormatter has also undergone some minor changes, including it here for completion:

public class FractionFormatter : IFormatProvider, ICustomFormatter
{
    private readonly CultureInfo _culture;

    public FractionFormatter(CultureInfo culture)
    {
        _culture = culture;
    }

    public static FractionFormatter Default
    {
        get { return new FractionFormatter(CultureInfo.CurrentUICulture); }
    }

    public object GetFormat(Type formatType)
    {
        return (formatType == typeof(ICustomFormatter)) ? this : null;
    }

    public string Format(string format, object arg, IFormatProvider formatProvider)
    {
        var fraction = (Fraction)arg;
        if (string.IsNullOrEmpty(format))
        {
            return string.Format(_culture, "{0}/{1}", fraction.Numerator, fraction.Denominator);
        }

        var result = string.Format(_culture, "{0:" + format + "}", fraction.ToFloat());
        return result;
    }
}

Same with MathJaxFractionFormatter:

public class MathJaxFractionFormatter : IFormatProvider, ICustomFormatter
{
    public enum MathJaxFractionSize
    {
        Normal,
        Large
    }

    private static readonly CultureInfo _culture = typeof(FractionFormatter).Assembly.GetName().CultureInfo;

    private readonly string _delimiter;
    private readonly MathJaxFractionSize _size;

    public MathJaxFractionFormatter()
        : this("$", MathJaxFractionSize.Normal) { }

    public MathJaxFractionFormatter(string delimiter, MathJaxFractionSize size)
    {
        _delimiter = delimiter;
        _size = size;
    }

    public object GetFormat(Type formatType)
    {
        return (formatType == typeof(ICustomFormatter)) ? this : null;
    }

    public string Format(string format, object arg, IFormatProvider formatProvider)
    {
        var fraction = (Fraction)arg;
        if (string.IsNullOrEmpty(format))
        {
            var keyword = _size == MathJaxFractionSize.Normal ? "\\frac" : "\\dfrac";
            return string.Format(_culture, "{2}{3}{{{0}}}{{{1}}}{2}", fraction.Numerator, fraction.Denominator, _delimiter, keyword);
        }

        return fraction.ToString(format, _culture);
    }
}

Now this will print a culture-sensitive 33.333 %:

Console.WriteLine(new Fraction(1, 3).ToString("p3", new MathJaxFractionFormatter()));

And this will print $\frac{1}{3}$ as expected:

Console.WriteLine(new Fraction(1, 3).ToString(new MathJaxFractionFormatter()));

Passing in a CultureInfo.InvariantCulture will simply cause ToString to use FractionFormatter.Default.

Console.WriteLine("5/25 with InvariantCulture: {0}", 
                  new Fraction(5, 25).ToString(CultureInfo.InvariantCulture));

Outputs 5/25 with InvariantCulture: 5/25.

share|improve this question
    
That's actually all of the code, not just a fraction of it ;) –  Mat's Mug Jul 14 at 4:26
    
Holy cow, Batman. That's a lot of code! –  Phrancis Jul 14 at 4:33
add comment

2 Answers 2

up vote 11 down vote accepted

I'd second mjolka's comments that the behaviour of ++ and -- is confusing, and that 0 would be preferable to default(int)

NaN and infinity

You have a single case for a zero denominator called IsUndefined. By comparison, Single has IsInfinity, IsPositiveInfinity, IsNegativeInfinity and IsNaN. Likewise, it has static NaN, NegativeInfinity and PositiveInfinity constants.

For consistency, I would consider renaming Empty to NaN, and splitting IsUndefined into three cases, for the numerator < 0 (negative infinity), == 0 (NaN) and > 0 (positive infinity). These can be used when converting to and from float.

Parsing

You should use int.TryParse rather than int.Parse. For example, if the numerator or denominator is larger than int.Max, you want to return false rather than throwing an exception.

In32.TryParse (and similar methods) actually guarantee a particular value to their out parameter. It's hard to think of a situation where this is important, but if you wanted to be super-conscietious you might consider doing the same. Note that unlike most numeric types, default(Fraction) is actually not Fraction.Zero but Fraction.Empty. You might want to think about which of these you'd rather return- 0/1 for consistency with other numeric types, or 0/0 for consistency with default. And either way, you could state your decision explicitly in your xml comments.

You might also consider having a Parse method like other numeric types. It's not crucial, but it saves the consumer some work in the (common) situation that an invalid input is considered exceptional.

Simplification

It seems a little unpredictable when fractions get simplified. I wouldn't necessarily expect that I could define a fraction 2/4 and it would remain that way until I multiplied it by 1, at which point it would become 1/2. Especially by making Simplify public, I think you imply that simplification is something should work in a way understandable to a consumer, without having to read through all your method implementations.

You could go through and try to use it consistently, but I think the simplest way would be to do the simplification inside the constructor so that all fractions are guaranteed to always be simplified. I can't think of a convincing reason that you'd want to work with unsimplified fractions. In this case you'd want to make methods relating to simplification private. And obviously they would need to take a numerator and denominator as parameters rather than fractions.

Avoidable overflows

This maybe falls into the category of "nice to have", but you should try to ensure you don't hit overflows inside arithmetic operations when it's avoidable. For example, if you have a defined as int.Max/2 + 1, then a/2 * 2/a would overflow, when it should equal 1/1.

For multiplication, if you have a/b * c/d, both already simplified, then the easiest way to do this would be to first define and simplify a/d and c/b, then multiply those.

I don't think there's any improvement that you can make for addition as long as the input fractions are already simplified. EDIT: Actually, one thing you can do is instead of using the product of the two denominators, instead use the least common multiple. You'll need to work out what to multiply the two numerators by before adding them. This one is actually probably more important than multiplication because even adding 1/50000 + 1/50000 will overflow as it is now.

Comparison

According to msdn documentation for IComparable.CompareTo:

The parameter, obj, must be the same type as the class or value type that implements this interface; otherwise, an ArgumentException is thrown.

I can see why you might want to support comparing to an integer, but comparing to a string really seems like overkill. It's also potentially dangerous, as it doesn't adhere to all of these rules from the same article:

A.CompareTo(A) must return zero.

If A.CompareTo(B) returns zero, then B.CompareTo(A) must return zero.

If A.CompareTo(B) returns zero and B.CompareTo(C) returns zero, then A.CompareTo(C) must return zero.

If A.CompareTo(B) returns a value other than zero, then B.CompareTo(A) must return a value of the opposite sign.

If A.CompareTo(B) returns a value x not equal to zero, and B.CompareTo(C) returns a value y of the same sign as x, then A.CompareTo(C) must return a value of the same sign as x and y.

Since strings compare alphabetically, it's easy to imagine a combination of two strings and a fraction which would compare inconsistently. e.g.:

var quarter = "1/4";
var third = new Fraction(1,3);
var half = "1/2";
Console.Out.WriteLine(quarter.CompareTo(half));  // 1
Console.Out.WriteLine(third.CompareTo(half));    // -1
Console.Out.WriteLine(third.CompareTo(quarter)); // 1

GetHashCode

As it is, I it would be possible to get two instances where Equals returns true but which have a different hash code due to float rounding. I did a quick test and I believe that 1/3 would have a different hash code to 5592409/16777227, for example.

Enforcing that all fractions are always simplified as I described before would fix this. Alternatively you could use, Tuple.Create(_numerator, _denominator).GetHashCode(), though I'm not sure about the performance impact of this. You could also look into formulae for generating hash codes from two integers. Or, for explicit consistency with the Equals method, you could find the long product of the numerator and the denominator and take the hash code of that.

IConvertable

For added support, you might consider implementing the IConvertable interface. This is relatively straightforward and probably best demonstrated in the linked article. For specific types such as Int32, Int64 and Boolean you might want to implement your own conversion, but otherwise, the simplest approach is to convert first to a common numeric type (probably float) then call the corresponding IConvertable method on the result.

share|improve this answer
    
Wow.. determining which answer to accept was hard! I chose yours because it made me realize I had somewhat forgotten about the immutable nature of my type; simplifying in the constructor is very neat! And you're right about everything else, too. Nice answer! –  Mat's Mug Jul 15 at 4:25
add comment

It looks good! Here are some issues I found.

Readability

I would recommend 0 over default(int) for readability.

Naming

Greatest common divisor is a more standard term for greatest common denominator.

MinValue

Fraction.MinValue should be new Fraction(int.MinValue, 1), not new Fraction(1, int.MinValue).

Immutability

Since your data structure is immutable, this code

public static Fraction Simplify(Fraction fraction)
{
    if (fraction.IsUndefined)
    {
        return new Fraction(fraction);
    }
    ...

can be

public static Fraction Simplify(Fraction fraction)
{
    if (fraction.IsUndefined)
    {
        return fraction;
    }
    ...

++ and --

From MSDN:

The increment operator (++) increments its operand by 1.

However, the way the increment and decrement operators are written, they increment (decrement) by 1 / denominator. This leads to confusing behaviour.

For example, how can we make this assertion fail?

if (x == y)
{
    x++;
    y++;
    Debug.Assert(x == y);
}

Just set

var x = new Fraction(2, 4);
var y = x.Simplify();

After incrementing, we have x = 3/4 and y = 2/2.

Or this contrived example will fail for x = new Fraction(1, 2):

var y = x;
// Waste time by doing nothing to x.
x++;
x *= 1;
x--;
// Make sure we didn't change x.
Debug.Assert(x == y);

I would stick to the guidelines and increment (decrement) by 1.

TryParse

You probably want to use the character class [0-9] instead of \d in your regular expression. This is because \d matches Unicode decimal digits. So this code will throw a FormatException:

Fraction f;
if (Fraction.TryParse("١/٢", out f))
{
    Console.WriteLine(f);
}

Floats

Comparing to floats is... tricky.

float oneThird = 1.0f / 3;
Console.WriteLine(new Fraction(1, 3) == oneThird);
> False

(Oh, and good call on not using #region.)

share|improve this answer
    
Try Console.WriteLine(new Fraction(1, 3) == 1f/3f); instead.. wait.. ==1f/3 returns true here... –  Mat's Mug Jul 14 at 5:05
    
@Mat'sMug I'm still getting false for Console.WriteLine(new Fraction(1, 3) == 1f/3f);. Are you sure you're using the code as posted here? –  mjolka Jul 14 at 5:07
    
ah, I have changed the == operator that involves a Fraction and a float, to return fraction.ToFloat().Equals(value); –  Mat's Mug Jul 14 at 5:08
    
Please expand on "increment/decrement operators are broken", I intended {1/2}++ to return {2/2}.. like, var oneThird = new Fraction(1,3); oneThird++; - oneThird is now 2/3. –  Mat's Mug Jul 14 at 5:10
1  
@Mat'sMug - x++ should always be the same as x = x+1. I didn't dig into your code to see if that's true, but if it is, great, and if not, you're likely to confuse people. It's not wrong if you document it as meaning something different (for that matter, you can have a class that supports ++ and not +), but it could be confusing. –  Bobson Jul 14 at 20:45
show 3 more comments

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.