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.

Inspired by this question: Fraction (rational number) structure with custom operators, I have written this class for doing some simple work with fractions.

Fraction.h:

#import <Foundation/Foundation.h>

@interface Fraction : NSObject

@property int numerator, denominator;

-(void) print;
-(void) setNumerator: (int) n;
-(void) setDenominator: (int) d;
-(void) setTo: (int) n over: (int) d;
-(double) convertToNum;
-(void) add: (Fraction*) f;
-(void) subtract: (Fraction*) f;
-(void) multiply: (Fraction*) f;
-(void) divide: (Fraction *) f;
-(void) reduce;

@end

Fraction.m:

#import "Fraction.h"

@implementation Fraction

@synthesize numerator, denominator;

-(void) print
{
    NSLog(@"%i/%i", numerator, denominator);
}

-(void) setNumerator: (int) n
{
    numerator = n;
}

-(void) setDenominator: (int) d
{
    denominator = d;
}

-(void) setTo: (int) n over: (int) d
{
    numerator = n;
    denominator = d;
}

-(double) convertToNum
{
    if (denominator) return (double) numerator / denominator;
    else return NAN;
}

-(void) add: (Fraction*) f
{
    numerator = numerator * f.denominator + denominator * f.numerator;
    denominator *= f.denominator;

    [self reduce];
}

-(void) subtract: (Fraction*) f
{
    numerator = numerator * f.denominator - denominator * f.numerator;
    denominator *= f.denominator;

    [self reduce];
}

-(void) multiply: (Fraction *) f
{
    numerator *= f.numerator;
    denominator *= f.denominator;

    [self reduce];
}

-(void) divide: (Fraction *) f
{
    numerator *= f.denominator;
    denominator *= f.numerator;

    [self reduce];
}

-(void) reduce
{
    int u = numerator;
    int v = denominator;
    int temp;

    while (v)
    {
        temp = u % v;
        u = v;
        v = temp;
    }

    numerator /= u;
    denominator /= u;
}

@end

main.m:

#import "Fraction.h"

int main(int argc, const char* argv[])
{
    @autoreleasepool
    {
        Fraction *frac1 = [[Fraction alloc] init];
        Fraction *frac2 = [[Fraction alloc] init];

        [frac1 setTo: 1 over: 3];
        [frac2 setTo: 4 over: 5];
        [frac1 add: frac2];
        [frac1 subtract: frac2];
        [frac1 multiply: frac2];
        [frac1 divide: frac2];
        NSLog(@"%g", [frac1 convertToNum]);
        NSLog(@"%g", [frac2 convertToNum]);
    }
}

Is there anything that I'm doing blatantly wrong? Are there some set practices I am not abiding by? Anything that I'm missing?

share|improve this question
add comment

5 Answers 5

up vote 6 down vote accepted

In Objective-C value objects are typically immutable (NSValue, NSNumber, and NSDecimalNumber are all immutable). I would recommend making your Fraction class immutable to be consistent with the existing convention. If you did this, your methods like add would take a Fraction and also return a new one.

Part of the reason immutability is important for "value" objects is that people don't typically expect them to change. With NSNumber, I can declare a variable:

NSNumber *five = @5; 

and pass it around to methods, with the confidence that it doesn't change. With your Fraction class, any method I passed my Fraction object to could change its numerator and denominator, making it less safe to use.

If you made it immutable, you would also need to add an initializer that created the numerator and denominator at initialization time, since you couldn't modify the object after creation. Regardless of immutability, I'd recommend creating such an initializer for convenience.

Its not idiomatic to create a print method. Instead, override the description method. This way, when someone does NSLog(@"%@",myFraction) it will call into the description method and style it nicely.

Per Apple's guidelines, avoid abbreviating method signatures, as in convertToNum.

Your method signatures are written a bit un-idiomatically; I would write them like this:

- (void)subtract:(Fraction *)fraction;

You can see this pattern in Apple's code as referenced in the link above, or by looking at Apple's framework header files (command click a class like NSNumber to see its header file in Xcode)

share|improve this answer
1  
While it's true that the Foundation classes come in mutable and immutable pairs, it's not strictly always necessary. Mutable subclassing causes a lot of problems when you're trying to then subclass that.. but you can only subclass either the mutable or immutable class... –  nhgrif Jul 13 at 0:55
1  
With that said though, it's still a good point to make and a good first answer. –  nhgrif Jul 13 at 0:58
add comment

@property

You declare properties in the header file, but you don't actually use them as anything more than instance variables, which suggests you may not quite understand their full power.

An Objective-C property is three things.

  1. An instance variable
  2. A setter
  3. A getter

When you write @property int numerator;, you have created all three of these things.

Whether or not you write anything else, the class now has a private instance variable known as _numerator, a getter method in the form of - (int)numerator; and a setter method in the form of - (void)setNumerator:(int)numerator;

So given a fraction variable called myFrac, all of the following is instantly valid:

myFrac.numerator = 3;
NSLog(@"%i", myFrac.numerator);
[myFrac setNumerator: 4];
NSLog(@"%i", [myFrac numerator]);

The square bracket notation is perfectly valid, though when dealing with properties in Objective-C, we tend to prefer just using the dot notation.

What's more is that this fact makes these methods superfluous:

-(void) setNumerator: (int) n
{
    numerator = n;
}

-(void) setDenominator: (int) d
{
    denominator = d;
}

Except... not completely. The first one is definitely unnecessary. The second, however... it needs some logic. Zero is not a valid denominator. We need to prevent the case of the user setting the denominator to zero. I think this is one of the rare cases where throwing an exception in Objective-C might be okay. And in fact, why don't we just throw the exact exception that'd be thrown if we did actually divide by zero?

- (void)setDenominator:(int)denominator {
    if (denominator == 0) {
        // no reason to build a NSException object, just:
        __unused int divisionByZero = numerator/denominator;
    }
    _denominator = denominator;
}

I mean, I see that in converting it to a double, you deal with this, but what about when you print? You really shouldn't let the object exist in an invalid state.

And you might have noticed I used a mysterious _denominator there. As of Xcode 4 (I think, maybe even older), properties are autosynthesized in the .m file to their name prefixed with an underscore. The @synthesize directive is no longer needed, and all Objective-C developers I know have abandoned it in favor of using the autosynthesized version. The underscored name also is a huge clue that this is a direct access instance variable and not anything else.


-(void) setTo: (int) n over: (int) d
{
    numerator = n;
    denominator = d;
}

You follow this pattern of using short names for the arguments, and I think it's probably simply to avoid the name class with your synthesized property names. And now you've just discovered another reason why the auto-synthesized underscore prefixed property names are great! Although, truthfully, we should be going through the setters, so this method should look like this:

- (void)setTo:(int)numerator over:(int)denominator {
    self.numerator = numerator;
    self.denominator = denominator;
}

And if you put an NSLog statement in setDenominator:, you'll see that this method will now go through that method to set the value.

By the way, notice how I keep moving your opening braces to the correct line?


-(void) print
{
    NSLog(@"%i/%i", numerator, denominator);
}

As far as I know, methods like this, across the board, are a no-no in OOP. Instead, classes should have an instant method which returns a string to be printed by whoever is using the class if they want to print the class. In most languages, the method is called toString(), in Objective-C, we call it description. Your description method should look like this:

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

One thing to note is the behavior of NSLog. NSLog doesn't just work for NSString objects. It works any object. As all Objective-C objects have a description method (which at a minimum is inherited from their super class), NSLog prints the value returned by the object's description method. As such, by implementing description, see what happens if you call:

NSLog(@"%@", myFrac);

-(double) convertToNum
{
    if (denominator) return (double) numerator / denominator;
    else return NAN;
}

This method is actually problematic for me. There's no telling what type of variable someone might need out of your fraction. Whose to say they don't need a float? Or an int? Or an NSInteger? There are a lot of possible types of number data types that one might need...

So, for starters, I'll point you to other Objective-C classes with method names like doubleValue, intValue, integerValue, etc. As such, a method that returns the double representation of an object should probably be called doubleValue.

But perhaps what may be even more important that writing a method for all the different numeric data types... why don't we make use of Objective-C class categories and the NSNumber class?

In the .h file for this class, add the following:

@interface NSNumber (Fraction) 

+ (NSNumber *)numberWithFraction:(Fraction *)fraction;

@end

And now in the .m file, assuming you've properly changed over to a method called doubleValue, add the following:

@implementation NSNumber (Fraction)

+ (NSNumber *)numberWithFraction:(Fraction *)fraction {
    return [NSNumber numberWithDouble:[fraction doubleValue]];
}

@end

Now anyone who has imported this file has also imported the ability to seemlessly create NSNumber objects with a single method call, and it will feel just like creating a NSNumber object with any other data type.

share|improve this answer
2  
I don't understand this line: "no reason to build a NSException object" followed by doing a division by zero. That won't actually throw an exception; it's behavior is undefined, though for me NSLog(@"%i",1/0); prints 1. –  MaxGabriel Jul 13 at 4:30
    
Well hrmph, though that threw an exception. If not, guess we need to create and throw the exception. –  nhgrif Jul 13 at 13:07
    
Prefix that NSNumber category method, please. –  Josh Caswell Jul 13 at 18:31
    
@nhgrif Did you mean to make that category method a class method? That would make more sense as its initializing a new instance and not referencing self. –  MaxGabriel Jul 14 at 1:05
    
Yes. Good catch. Fixed it. –  nhgrif Jul 14 at 11:18
add comment

Obj-C is not my strong suit, so I thought about putting this in a comment instead. But I guess it's an answer of sorts. It's all about naming, though

  • When saw the name convertToNum I first expected it'd return an NSNumber instance. Besides, I think most Obj-C APIs would avoid abbreviations like "num". Perhaps doubleValue would be better? Perhaps declare doubleValue as a read-only property while you're at it.

  • Of course, it might be easier to just return an NSNumber somewhere. Then the caller can get doubleValue (and other types) from that instead.

  • I like the setTo:over: signature, but it makes me think multiply: and divide: should be multiplyBy: and divideBy: to be equally "plain English"-sounding.

share|improve this answer
1  
multiplyBy and divideBy sound good. –  nhgrif Jul 13 at 0:03
add comment

In addition to everything in my other answer, I want to address some things that are missing and I feel like should be included.

init

You have NO init methods. Are you crazy!?!?

It's not necessarily always important to include init methods, but in this case, I definitely think we should offer both init and factory methods.

In case your wondering, a factory method is a convenience method for instantiating an object and it removes the need for alloc.

We should implement two init methods, and handle everything else through factory methods.

First, here's what the basic init pattern looks like in Objective-C:

- (instancetype)init {
    self = [super init];
    if (self) {
        // initialize
    }
    return self;
}

So with that in mind, what might init methods for this class look like?

Well, we have to implement init in a way which leaves our object in a functional state but we'll also want an init that takes an argument for each of our properties. So, here's what that might look like:

- (instancetype)initWithNumerator:(int)numerator denominator:(int)denominator {
    if (denominator == 0) {
        numerator/denominator;
    }
    self = [super init];
    if (self) {
        _numerator = numerator;
        _denominator = denominator;
    }
    return self;
}

- (instancetype)init {
    return [self initWithNumerator:0 denominator:1];
}

So now that we've got our init methods in place, let's write some factory methods that will really make initialization nice:

+ (instancetype)zero {
    return [[self alloc] init];
}

+ (instancetype)one {
    return [[self alloc] initWithNumerator:1 denominator:1];
}

+ (instancetype)fractionWithInteger:(int)integer {
    return [[self alloc] initWithNumerator:integer denominator:1];
}

+ (instancetype)fractionWithNumerator:(int)numerator denominator:(int)denominator {
    return [[self alloc] initWithNumerator:numerator denominator:denominator];
}

So now what have we done?

Well, now instead of:

Fraction *zero = [[Fraction alloc] init];

We can just do:

Fraction *zero = [Fraction zero];

And instead of:

Fraction *oneHalf = [[Fraction alloc] init];
oneHalf.numerator = 1;
oneHalf.denominator = 2;

We can just do this:

Fraction *oneHalf = [Fraction fractionWithNumerator:1 denominator:2];

Isn't this much nicer?


There's another something important missing. Your math functions are nice and all, but they're purely equivalent to compound assignment operators. Could you imagine if we were only able to use +=, -=, *=, and /=? So, let's fix that.

We need to also be sure to include methods for adding our fractions that leave the originals untouched but return a new value.

That'd look something like this:

- (Fraction *)fractionByAddingFraction:(Fraction *)fraction {
    int newNumerator = self.numerator * fraction.denominator + self.denominator * fraction.numerator;
    int newDenominator = self.denominator * fraction.denominator;

    return [Fraction fractionWithNumerator:newNumerator denominator:newDenominator];
}

And repeat for all the other operations. Using this function would look like this:

Fraction *threeFourths = [oneHalf fractionByAddingFraction:oneQuarter];
share|improve this answer
add comment

Hide the implementation details

I think the numerator and denominator properties should be readonly from the outside and readwrite on the inside of the class. There should be no way for calling code to change the numerator or denominator individually. Allowing this makes it too easy to end up with an invalid rational number. (For some operations, knowing that a fraction is already reduced allows better implementation, so you might decide that a "valid" fraction must always be reduced.)

And what if you wanted (for some weird reason) to represent rational numbers as continued fractions instead of just storing the numerator and denominator?

Two motivations for wrapping code in a class like this are:

  1. Make it impossible (or at least hard) for client code to end up with an invalid object.
  2. Isolate the implementation details, so you can change them later without breaking client code.

Without these motivations, you might as well just use a bare struct and a bunch of non-member functions.

These aren't Objective-C suggestions, they would be valid for nearly any object-oriented language.

In Objective-C, you can do this as follows. In the .h file:

@interface Fraction : NSObject
@property (readonly) int numerator;
@property (readonly) int denominator;
...
@end

In the .m' file, use a "class extension" to change the properties toreadwrite`:

@interface Fraction : ()
@property (readwrite) int numerator;
@property (readwrite) int denominator;
@end

readwrite is the default for properties, so it is often left out of the code. But I like to show it explicitly, for emphasis, when I'm redeclaring a property.

Reduce fractions sooner

This will avoid overflow problems in some cases, so your class will give correct results for a wider range of inputs.

For example, in add, try something like this:

int lcm = leastCommonMultiple(self.denominator, f.denominator);
self.numerator = (self.numerator * (lcm / self.denominator)) + 
                 (f.numerator    * (lcm / f.denominator));
self.denominator = lcm;
[self reduce];

Note that lcm divided by either denominator is an integer, so the math is exact.

Test case: make a fraction with numerator equal to 0.75 * MAX_INT (truncated to an int) and denominator equal to MAX_INT. Subtract the fraction from itself. Do you get zero, without any overflow?

Check the implementation of reduce

If my memory is right, the % operator in C has implementation-defined behavior if either operand is negative. The usual convention for rational numbers is to have the numerator carry the sign, and have the denominator always positive. Does your implementation do that, for all possible inputs?

share|improve this answer
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.