2
\$\begingroup\$

I decided to train myself in OOP with a simple perfect precision Fraction class.

from __future__ import division
import doctest


class Fraction:
    """
    Implements a Fraction Class with perfect precision
    operations and utility methods. The built-in operators
    are overwritten to provide a more natural interface.
    """
    def __init__(self,num,den):
        if den == 0:
            raise ValueError("Denominator must not be zero.")

        self.num = num
        self.den = den

    @staticmethod
    def greatest_common_divisor(a,b):
        """
        Returns the greatest number 'n' existing such
        that a % n == 0 and b % n == 0.
        This number may be one if 'a' and 'b' are coprimes.

        >>> Fraction.greatest_common_divisor(20,15)
        5
        """
        def common(a,b):
            return [i for i in a if i in b]

        def div(n):
            return [i for i in range(1,n+1) if n % i == 0]

        return max(common(div(a),div(b)))

    @staticmethod
    def invert(fraction):
        """
        Returns a fraction where the numerator is the previous
        denominator and vice-versa.

        >>> Fraction.invert(Fraction(3,5))
        5/3
        """
        return Fraction(fraction.den,fraction.num)

    def from_string(text):
        """
        Generates a Fraction object from a string rapresentation
        of two integers seperated by '/'.

        >>> Fraction.from_string('4/9') + Fraction.from_string('2/18')
        5/9
        """
        return Fraction(*[int(i) for i in text.split('/')])

    def simplify(self):
        """
        Returns an eqivalent but simpler Fraction.

        >>> Fraction.simplify(Fraction(210,20))
        21/2
        """
        fact = self.greatest_common_divisor(self.num,
                                       self.den)
        return Fraction(self.num // fact, self.den // fact)

    def __mul__(self,fraction):
        """
        Fraction multiplication.

        >>> Fraction(4,3) * Fraction(1,20)
        1/15
        """
        return Fraction.simplify(
            Fraction(self.num*fraction.num,
                       self.den*fraction.den))

    def __add__(self,fraction):
        """
        Fraction addition.

        >>> Fraction(4,9) + Fraction(11,7)
        127/63
        """
        common_den = self.greatest_common_divisor(
            self.den,fraction.den)
        num1 = self.num * fraction.den
        num2 = fraction.num * self.den
        return Fraction.simplify(
            Fraction(num1+num2, fraction.den*self.den))

    def __sub__(self,fraction):
        """
        Fraction subtraction.

        >>> Fraction(1,2) - Fraction(1,3)
        1/6
        """
        return self + Fraction(-fraction.num,
                                fraction.den)

    def __truediv__(self,fraction):
        """
        Fraction division.

        >>> Fraction(4,8) / Fraction(9,2)
        1/9
        """
        return self * self.invert(fraction)

    def __repr__(self):
        """
        Returns a printable representation of the fraction
        that can also be fed back into the class via
        'Fraction.from_string'.
        This method is called automatically on printing.

        >>> Fraction(5,8)
        5/8
        >>> Fraction(2,9)
        2/9
        """
        return '{}/{}'.format(self.num, self.den)

def main():
    doctest.testmod()

if __name__ == "__main__":
    main()
\$\endgroup\$
4
\$\begingroup\$

The from_string() function is missing a @staticmethod decorator, and thus does not work in Python 2.


Your greatest_common_divisor() function works using brute force: it literally tests every possible divisor and picks the greatest one that is common to both. A much faster method is the Euclidean Algorithm.

@staticmethod
def gcd(a, b):
    while b:
        a, b = b, a % b
    return a

Validation and error handling could be tightened up a bit.

  • The rejection of floating-point arguments is inconsistent:

    >>> Fraction(3.14, 1)
    3.14/1
    >>> Fraction.from_string('3.14/1')
    Traceback (most recent call last):
      […]
      File "fraction.py", line 55, in <listcomp>
        return Fraction(*[int(i) for i in text.split('/')])
    ValueError: invalid literal for int() with base 10: '3.14'
    
  • Instantiating a Fraction with strings initially appears to work, but fails in an odd way later. Either both of these statements should work, or both should fail.

    >>> Fraction('1', '3')
    1/3
    >>> Fraction('1', '3') + Fraction('2', '3')
    Traceback (most recent call last):
      […]
      File "fraction.py", line 32, in div
        return [i for i in range(1,n+1) if n % i == 0]
    TypeError: Can't convert 'int' object to str implicitly
    
  • In Fraction.from_string(), the abstraction is slightly leaky. I would prefer to see a ValueError instead of the following:

    >>> Fraction.from_string('1/3/5')
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "fraction.py", line 55, in from_string
        return Fraction(*[int(i) for i in text.split('/')])
    TypeError: __init__() takes 3 positional arguments but 4 were given
    
  • Personally, I would choose to raise a ZeroDivisionError if the denominator is 0.

    >>> Fraction(1, 0)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "fraction.py", line 13, in __init__
        raise ValueError("Denominator must not be zero.")
    ValueError: Denominator must not be zero.
    
\$\endgroup\$
3
\$\begingroup\$

From the docs:

[__repr__] should look like a valid Python expression that could be used to recreate an object with the same value (given an appropriate environment).

Your current __repr__ should really be renamed __str__, and replaced by something like:

 def __repr__(self):
     return 'Fraction({0.num}, {0.den})'.format(self)

invert needs access to the class, so should probably be a @classmethod rather than @staticmethod:

@classmethod
def invert(cls, fraction):
    """
    Returns a fraction where the numerator is the previous
    denominator and vice-versa.

    >>> Fraction.invert(Fraction(3, 5))
    5/3
    """
    return cls(fraction.den, fraction.num)

This factors out the explicit class name from the function, making it work better with any future inheritance. Note that I've also added some extra spaces; see the style guide. You could alternatively implement it as a standard instance method, then call it like Fraction(3, 5).invert().

Similarly, you can remove the explicit class from from_string by making it a class method (currently it's an instance method without a self parameter, so won't work in Python 2.x):

@classmethod
def from_string(cls, text):
    """
    Generates a Fraction object from a string rapresentation
    of two integers seperated by '/'.

    >>> Fraction.from_string('4/9') + Fraction.from_string('2/18')
    5/9
    """
    return cls(*[int(i) for i in text.split('/')])

You can also simplify with e.g. cls(*map(int, text.split('/')).

Where you return a new Fraction from an instance method, you can access the class less explicitly with self.__class__(...).


You don't need to implement greatest_common_divisor yourself; Python has fractions.gcd.

\$\endgroup\$
  • 3
    \$\begingroup\$ Arguably, reusing fractions.gcd is cheating. You might as well reuse the entire fractions.Fraction class. \$\endgroup\$ – 200_success Mar 6 '15 at 3:05
  • \$\begingroup\$ @200_success fair point! But if the objective was learning OOP, implementing gcd isn't really necessary. \$\endgroup\$ – jonrsharpe Mar 6 '15 at 7:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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