patterncsharpMinor
Formattable Fraction
Viewed 0 times
formattablefractionstackoverflow
Problem
I have written this small little program, to test how my
```
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
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, so2/5could 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:
Edge cases
As @nhgrif hinted at, you should throw an
As @nhgrif mentioned, you need to be careful handling zero denominators. For example,
Recursion
Culture
Why are you getting the
API
Add a
You might also consider providing
Comparing fractions
Though I can't say it's wrong, it does feel weird to use floating-point values to compare fractions:
I would expect something like this:
Then,
could be
Nit-picking
Finally, though it's a matter of preference, I would remove the redundant
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/1Recursion
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.