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.

Here is my Python implementation of a simple fractions class:

class frac:
    def __init__(self, a,b):
        self.a = a
        self.b = b

    def __add__(self, x):
        return frac(self.a*x.b + self.b*x.a, self.b*x.b)    

    def __mul__(self, x):
        return frac(self.a*x.a, self.b*x.b)   

    def __sub__(self, x):
        return frac(self.a*x.b - self.b*x.a, self.b*x.b) 

    def __div__(self, x):
        return frac(self.a*x.b, self.b*x.a) 

    def __repr__(self):
        return "%s/%s"%(self.a,self.b)

It does addition OK:

x = frac(1,3)
y = frac(1,2)
print x+ y

>> 5/6

However, it can miss the mark:

x = frac(1,3)
y = frac(2,3)
print x+ y

>> 9/9

It seems if I run this library enough, the numerators and denominators will get very large. When should I start simplifying and where should I put that code?


There seems to be a fractions class already in Python, it this was instructive to put together.

My original goal was to allow for square roots of integers as well, but I will ask this in a separate question.

share|improve this question
    
You might consider accepcting the most helpful answer by clicking the check mark below the voting buttons. –  ojdo Apr 29 at 13:54
add comment

2 Answers 2

up vote 10 down vote accepted

I would probably put it right in your __init__ method, which simplifies it every time a fraction is created (which includes after any operation).

I disagree with Jaime's answer that you should do the simplification as part of the operation, as you'll have to copy/paste that code for every new operation.

I’d write something like:

def gcd(x, y):
    while y:
        x, y = y, x % y
    return x

class Fraction(object):
    def __init__(self, numer, denom):
        cancel_factor = gcd(numer, denom)
        self.numer = numer / cancel_factor
        self.denom = denom / cancel_factor

A few other comments:

  • Per PEP 8 on class names, I'm using Fraction instead of frac.

    Also from PEP 8, I would put spaces around your multiplication operators.

  • New style classes should inherit from object (as I've done above).

  • I think numer and denom are better attribute names than a and b.

  • The convention is to use other as the variable name in a method that takes two inputs, rather than x. For example:

    def __add__(self, other):
        new_numer = self.numer * other.denom + self.denom * other.numer
        new_denom = self.denom * other.denom
        return Fraction(new_numer, new_denom)
    
  • I disagree with your use of __repr__. As a rule of thumb, __repr__ should be something you can eval() to get back to that object; __str__ is the human-readable object. See difference between __str__ and __repr__ for more.

    Here's what I'd write:

    def __repr__(self):
        return "Fraction(%s, %s)" % (self.numer, self.denom)
    
    def __str__(self):
        return "%s / %s" % (self.numer, self.denom)
    
share|improve this answer
    
That's indeed a much better structuring of the code, +1. –  Jaime Apr 23 at 20:50
add comment

If you want to produce correct results, you should simplify after each operation. Euclid's algorithm is very fast in finding the GCD, and very simple to code. If you have a function like:

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

Then you could rewrite, e.g. you __add__ method as:

def __add__(self, other):
    num = self.a * other.b + self.b * other.a
    den = self.b * other.b
    gcf = gcd(num, den)

    return frac(num // gcf, den // gcf)
share|improve this answer
1  
This simplification is needed in each function (and maybe even outside) so it should be a function of its own. –  Nobody Apr 23 at 18:31
    
Euclid's algorithm may be fast compared with a naive approach, but if code reduces after every addition, subtraction, multiplication, or division, it will spend more time doing the reductions than it spends doing everything else combined. –  supercat Apr 23 at 21:00
1  
If you want correctness, you have to keep fractions in lowest terms. That the implementation in the standard Python library does exactly the same as @alexwlchan proposes, see here, is a good indicator that this is the proper way of going about it. –  Jaime Apr 23 at 23:12
add comment

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.