Rational class to handle fractions

Question

I am very new to Objective-C as well as objected oriented programming, and in a book I am studying from there is an exercise in which I was supposed to create a class called Rational, that has hidden data members called numerator and denominator, and methods to add, multiply, subtract and divide the Rational objects (the objects are just fractions) together. For some reason when I run the program it becomes extremely slow when calculating. I am using ARC on X-Code, and I am wondering if it has to do with memory management issues.

Here is the .m file of the class:

#import "Rational.h"
@interface Rational (privateMethods)

-(int) gcd:(int) a: (int) b;
-(Rational*) simplifyFraction:(Rational*)fraction;

@end


@implementation Rational

@synthesize numerator, denominator;

-(Rational*) multiplyFraction:(Rational *)fraction1 :(Rational *)fraction2{

    fraction1.numerator = fraction1.numerator * fraction2.numerator;

    fraction1.denominator = fraction1.denominator * fraction2.denominator;

    fraction1 = [self simplifyFraction:fraction1];

    return fraction1;
}

-(Rational*) addFraction:(Rational *)fraction1 :(Rational *)fraction2{

    Rational * returnFraction = [[Rational alloc] init];

    fraction1.numerator = fraction1.numerator*fraction2.denominator;

    fraction2.numerator = fraction2.numerator *fraction1.denominator;

    fraction1.denominator = fraction1.denominator*fraction2.denominator;

    fraction2.denominator = fraction1.denominator;

    returnFraction.numerator = fraction1.numerator + fraction2.numerator;

    returnFraction.denominator = fraction1.denominator;

    returnFraction = [self simplifyFraction:returnFraction];

    return returnFraction;

}

-(Rational*) subtractFraction:(Rational *)fraction1 :(Rational *)fraction2{

    Rational * returnFraction = [[Rational alloc] init];

    fraction1.numerator = fraction1.numerator*fraction2.denominator;

    fraction2.numerator = fraction2.numerator *fraction1.denominator;

    fraction1.denominator = fraction1.denominator*fraction2.denominator;

    fraction2.denominator = fraction1.denominator;

    returnFraction.numerator = fraction1.numerator - fraction2.numerator;

    returnFraction.denominator = fraction1.denominator;

    returnFraction = [self simplifyFraction:returnFraction];

    return returnFraction;
}

-(Rational*) divideFraction:(Rational *)fraction1 :(Rational *)fraction2{

    Rational * returnFraction = [[Rational alloc] init];

    const int temp = fraction2.denominator;

    fraction2.denominator = fraction2.numerator;

    fraction2.numerator = temp;

    returnFraction.numerator = fraction1.numerator * fraction2.numerator;

    returnFraction.denominator = fraction2.denominator * fraction1.denominator;

    returnFraction = [self simplifyFraction:returnFraction];

    return returnFraction;
}

-(void) printObject:(Rational *)fraction{

    printf("%i/%i",fraction.numerator, fraction.denominator);

    printf("\n");
}

-(void) printRoundedFloat:(Rational *)fraction{

    float number = (float)fraction.numerator/fraction.denominator;

    printf("%f", number);

    printf("\n");
}


-(int)gcd:(int)a :(int)b{

    if (b==0) {

        return a;

    }

    else

        return [self gcd:b :a%b];
}

-(Rational*) simplifyFraction:(Rational *)fraction{

    if (fraction.denominator == 0) {

        NSLog(@"ERROR: YOU CAN NOT HAVE ZERO IN THE DENOMINATOR");

}

    else{

        int i = fraction.numerator > fraction.denominator ? fraction.numerator:fraction.denominator;

        while (i>1) {

            if (fraction.numerator % i == 0 && fraction.denominator%i==0) {

                fraction.numerator/=i;

                fraction.denominator/=i;

            }

            --i;

        }

    }

    return fraction;

}

-(void) dealloc{
}

@end

Here is the Main.m file of the program:

import <Foundation/Foundation.h>

import "Rational.h"

int main (int argc, const char * argv[])
{

    @autoreleasepool {

        Rational * newFraction = [[Rational alloc] init];
        Rational * otherFraction = [[Rational alloc] init];

        newFraction.numerator = 1;
        newFraction.denominator = 25;
        printf("Fraction 1 is: ");
        [newFraction printObject:newFraction];


        otherFraction.numerator = 1;
        otherFraction.denominator = 5;
        printf("Fraction 2 is:  ");
        [otherFraction printObject:otherFraction];

        printf("\nThe Fractions Added togeher are: ");
        id number = [newFraction addFraction:newFraction :otherFraction];
        [number printObject:number];
        printf("Rounded: ");
        [number printRoundedFloat:number];

        printf("\nThe Fractions subtracted are: ");
        number = [number subtractFraction:newFraction :otherFraction];
        [number printObject:number];
        printf("Rounded: ");
        [number printRoundedFloat:number];

        printf("\nThe Fractions multiplied are: ");
        number = [number multiplyFraction:newFraction :otherFraction];
        [number printObject:number];
        printf("Rounded: ");
        [number printRoundedFloat:number];

        printf("\nThe Fractions divided are: ");
        number = [number divideFraction:newFraction :otherFraction];
        [number printObject:number];
        printf("Rounded:  ");
        [number printRoundedFloat:number];
    }

    return 0;

Answer 1

@interface Rational (privateMethods)

-(int) gcd:(int) a: (int) b;
-(Rational*) simplifyFraction:(Rational*)fraction;

@end

This really should be a class extension rather than a private category. A class extension means leaving the parenthesis out, and perhaps more importantly it means if we add a method to the interface here and don't implement it, the IDE will give us a warning. Although honestly, it probably simply shouldn't exist at all--and we don't even necessarily need to declare the methods here even.

The gcd function, which should be named greatestCommonDenominator instead, isn't necessarily specific to fractions. It can exist as a regular C function outside the class. And honestly, I don't have a problem with exposing the function in the header file.

Anyway, to make it a regular C-function outside the class, just remove all of the existing stuff outside the @interface and @implementation sections, and write the function as such (above the @interface):

int greatestCommonDenominator(int a, int b) {
    if (b==0) {
        return a;
    } else {
        return greatestCommonDenominator(b, a%b);
    }
}

---

The other method, simplifyFraction:, shouldn't take an argument. Instead, it should simplify self... the instance which called the method.

In fact, this logic should be applied to almost every method in your class. Our instances methods should use self in some way. If we're not using self, it should be a class method.

---

-(void) printObject:(Rational *)fraction{

    printf("%i/%i",fraction.numerator, fraction.denominator);

    printf("\n");
}

-(void) printRoundedFloat:(Rational *)fraction{

    float number = (float)fraction.numerator/fraction.denominator;

    printf("%f", number);

    printf("\n");
}

It's a pretty hard and fast rule in OOP that objects shouldn't "print" themselves. Instead, we return a string, and let whoever is using the object do with that string whatever it wants. Both of these methods should be removed, and instead, we can offer:

- (NSString *)description {
    return [NSString stringWithFormat:@"%i/%i", self.numerator, self.denominator];
}

And rather than returning some special format for the string representation of the float value, why don't we just turn it into a float value and return that value?

- (float)floatValue {
    return (float)self.numerator/(float)self.denominator;
}

We can also do this for double, and potentially for integer values as well.

Answer 2

One thing to note here is that for some reason you coded up a gcd function, but do not utilize it in your simplifyFraction method.

By definition of gcd, it returns the greatest common divisor. Therefore, to simplify a fraction, you need simply to divide both the numerator and denominator by the gcd(numerator, denominator).

This would simplify your code a bit, make it more readable, and probably increase its speed.

Also, your division method is only made more complicated by having code such as:

returnFraction.numerator = fraction1.numerator * fraction2.numerator;

when intuitively it should be:

returnFraction.numerator = fraction1.numerator * fraction2.denominator;

I would eliminate the temp variable and essentially duplicate the style of the multiplication method to fix this.

Answer 3

I just had a flash of inspiration, although possibly not related.

What happens if you change this method signature:

-(void) printObject:(Rational *)fraction{

    printf("%i/%i",fraction.numerator, fraction.denominator);

    printf("\n");
}

to

-(void) printObject
{
    printf("%i/%i",self.numerator, self.denominator);

    printf("\n");
}

And then

[newFraction printObject:newFraction];

to

[newFraction printObject];

And so on...

Seems odd that you're passing the object to a message that belongs to the object you're passing.

Answer 4

You shouldn't be modifying the objects passed to the methods. If the user does not expect this, it could cause strange behavior later.

-(void) addFraction:(Rational *)fraction1 :(Rational *)fraction2
{
    const int num1 = fraction1.numerator * fraction2.denominator;
    const int num2 = fraction2.numerator * fraction1.denominator;

    self.numerator = num1 + num2;
    self.denominator = fraction1.denominator * fraction2.denominator;

    [self simplifyFraction:self];
}