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

Formattable Fraction

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

Problem

I have written this small little program, to test how my Fraction type behaves when used with and without a custom FractionFormatter - I've implemented two custom formatters:

  • FractionFormatter, which simply puts the numerator at the left of a slash, and the denominator at the right ("{0}/{1}", fraction.Numerator, fraction.Denominator),



  • MathJaxFractionFormatter, pushes the customization a bit further by formatting the fraction as MathJax, so 2/5 could be rendered as \$\frac{2}{5}\$ which produces \$\frac{2}{5}\$ in CR posts.



```
static void Main(string[] args)
{
var fraction1 = new Fraction(2);
var fraction2 = new Fraction(2, 4);

Console.WriteLine("Fraction1: {0} (decimal: {1})", fraction1, fraction1.ToDecimal());
Console.WriteLine("Fraction2: {0} (decimal: {1})", fraction2, fraction2.ToDecimal());

Console.WriteLine("{0} + {1} = {2} (decimal: {3})", fraction1, fraction2, fraction1 + fraction2, (fraction1 + fraction2).ToDecimal());
Console.WriteLine("{0} - {1} = {2} (decimal: {3})", fraction1, fraction2, fraction1 - fraction2, (fraction1 - fraction2).ToDecimal());
Console.WriteLine("{0} {1} = {2} (decimal: {3})", fraction1, fraction2, fraction1 fraction2, (fraction1 * fraction2).ToDecimal());
Console.WriteLine("{0} / {1} = {2} (decimal: {3})", fraction1, fraction2, fraction1 / fraction2, (fraction1 / fraction2).ToDecimal());

var jaxFormatter = new MathJaxFractionFormatter();
var fraction3 = new Fraction(2, jaxFormatter);
var fraction4 = new Fraction(2, 4, jaxFormatter);

Console.WriteLine("Fraction3: {0} ({1})", fraction3, fraction3.ToString(null, new FractionFormatter()));
Console.WriteLine("Fraction4: {0} ({1})", fraction4, fraction4.ToString(null, new FractionFormatter()));

var crJaxFormatter = new MathJaxFractionFormatter("\\$", MathJaxFractionFormatter.MathJaxFractionSize.Large);
var fraction5 = new Fraction(2, crJaxFormatter);
var fraction6 = new Fraction(2, 4, crJaxForm

Solution

Immutable structs

Yay! ✓✓✓

Format provider

It seems odd to see a format provider in a constructor, and stranger still to store a format provider for every fraction instance. Think of the memory overhead -- imagine a use case where I perform calculations on millions of fractions, but only want to print the end result.

Whoops

This will not terminate:

Console.WriteLine(new Fraction(1, 1, CultureInfo.InvariantCulture));


Edge cases

As @nhgrif hinted at, you should throw an ArgumentOutOfRange exception in the constructor for a zero denominator.

As @nhgrif mentioned, you need to be careful handling zero denominators. For example,

Console.WriteLine(new Fraction(1) / new Fraction(1, 0));


0/1


Recursion

GetGreatestCommonDenominator can be written iteratively, instead of recursively.

Culture

Why are you getting the CultureInfo from the assembly? I'm not sure, but I think it would be better to use CultureInfo.CurrentCulture.

API

Add a ToString(IFormatProvider) method so you don't have to call it with a first argument of null.

You might also consider providing MaxValue, MinValue, Zero, and One fields.

Comparing fractions

Though I can't say it's wrong, it does feel weird to use floating-point values to compare fractions:

public int CompareTo(Fraction other)
{
    return ToDecimal().CompareTo(other.ToDecimal());
}


I would expect something like this:

public int CompareTo(Fraction other)
{
    // TODO: handle case where this and/or other have zero denominator.
    long lhs = _numerator * other._denominator;
    long rhs = other._numerator * _denominator;
    return lhs.CompareTo(rhs);
}


Then,

public bool Equals(Fraction other)
{
    return ToDecimal().Equals(other.ToDecimal());
}


could be

public bool Equals(Fraction other)
{
    return this.CompareTo(other) == 0;
}


Nit-picking

Finally, though it's a matter of preference, I would remove the redundant elses, e.g.

if (obj is int)
{
    return CompareTo(new Fraction((int)obj, 1));
}
else if (obj is string)
{
...

Code Snippets

Console.WriteLine(new Fraction(1, 1, CultureInfo.InvariantCulture));
Console.WriteLine(new Fraction(1) / new Fraction(1, 0));
public int CompareTo(Fraction other)
{
    return ToDecimal().CompareTo(other.ToDecimal());
}
public int CompareTo(Fraction other)
{
    // TODO: handle case where this and/or other have zero denominator.
    long lhs = _numerator * other._denominator;
    long rhs = other._numerator * _denominator;
    return lhs.CompareTo(rhs);
}
public bool Equals(Fraction other)
{
    return ToDecimal().Equals(other.ToDecimal());
}

Context

StackExchange Code Review Q#56904, answer score: 7

Revisions (0)

No revisions yet.