5
\$\begingroup\$

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;
\$\endgroup\$
  • 2
    \$\begingroup\$ 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/… \$\endgroup\$ – Russ C Apr 12 '12 at 3:08
  • \$\begingroup\$ 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.. \$\endgroup\$ – James Bender Apr 12 '12 at 12:59
5
\$\begingroup\$
@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.

\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – James Bender Apr 12 '12 at 18:31
  • 2
    \$\begingroup\$ 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? \$\endgroup\$ – JeremyP Apr 16 '12 at 15:54
  • \$\begingroup\$ We really shouldn't be printing objects. JeremyP is right--the correct solution is overriding description. \$\endgroup\$ – nhgrif Jul 22 '14 at 2:48
1
\$\begingroup\$

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];
}
\$\endgroup\$

Your Answer

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