Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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 extremly 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;
share|improve this question
2  
Memory management could be the issue here; but can you give us a clear definition of what you mean by 'it becomes extremely slow'. Is this during the first pass, just in specific messages, do you have to run it a few times ? Also, perhaps try just disabling arc and see what that does ? This question has instructions on how to do this: stackoverflow.com/questions/7837024/… – Russ C Apr 12 '12 at 3:08
Each method runs fine individually, but when I call them all in the main, it takes about 2 seconds for both the multiplication and division to compute.. I've tried disabling arc and adding release statements, but it is still slow. I also tried it on another computer to assure it is not a hardware issue. It seems strange to take so long since the program is so small.. – James Bender Apr 12 '12 at 12:59

migrated from stackoverflow.com Apr 14 '12 at 13:10

3 Answers

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.

share|improve this answer
Although this didn't speed it up at all, I really appreciate this. I am new to OOP, and your code helped me understand the concept of self a lot better. I didn't realize that self would refer to the object that had been initialized. Thanks. – James Bender Apr 12 '12 at 18:31
2  
In fact the original form of the method did not use self at all so should have been a class method. However, rather than having a method called printObject why not override description? – JeremyP Apr 16 '12 at 15:54

Thanks Very Much Russ.. Your previous code post helped me understand messages a lot more I have changed my implementation to the following, and it reduced the runtime tremendously. I think previously I had to many allocations and initializations of relational objects, especially for using ARC:

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

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

@end


@implementation Rational

@synthesize numerator, denominator;

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

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

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

    [self simplifyFraction:fraction1];

}

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

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

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

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

     fraction2.denominator = fraction1.denominator;

     self.numerator = fraction1.numerator + fraction2.numerator;

    self.denominator = fraction1.denominator;

    [self simplifyFraction:self];


}

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

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

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

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

    fraction2.denominator = fraction1.denominator;

    self.numerator = fraction1.numerator - fraction2.numerator;

    self.denominator = fraction1.denominator;

    [self simplifyFraction:self];

}

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

    const int temp = fraction2.denominator;

    fraction2.denominator = fraction2.numerator;

    fraction2.numerator = temp;

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

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

    [self simplifyFraction:self];

}

-(void) printObject{

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

    printf("\n");

}

-(void) printRoundedFloat{

    float number = (float)self.numerator/self.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;
}


@end
share|improve this answer
that's great news! – Russ C Apr 12 '12 at 22:05

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];
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.