HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Rational class to handle fractions

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
handleclassfractionsrational

Problem

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.

Solution

@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.

Code Snippets

@interface Rational (privateMethods)

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

@end
int greatestCommonDenominator(int a, int b) {
    if (b==0) {
        return a;
    } else {
        return greatestCommonDenominator(b, a%b);
    }
}
-(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");
}
- (NSString *)description {
    return [NSString stringWithFormat:@"%i/%i", self.numerator, self.denominator];
}
- (float)floatValue {
    return (float)self.numerator/(float)self.denominator;
}

Context

StackExchange Code Review Q#10887, answer score: 5

Revisions (0)

No revisions yet.