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.

I have a simple Model class that represents a calculator.

Here is it's header file:

@class MMCalculator;

@protocol MMCalculatorDelegate <NSObject>

@required - (void)mmCalculator:(MMCalculator *)mmCalulatorFinishedCalculatingValues;

@end

@interface MMCalculator : NSObject

@property (nonatomic, weak) id <MMCalculatorDelegate> delegate;
@property float calculatedPay;
@property float calculatedSavingsForStuff;
@property float calculatedSavingsForProfitFormula;
@property float calculatedSavingsForTaxes;

- (void)calculateValuesWithMonthlyRevenue:(float)monthlyRevenue;
- (float)calculateYourPay:(float)monthlyRevenue;
- (float)calculateSavingsForStuff:(float)monthlyRevenue;
- (float)calculateSavingsForProfitFormula:(float)monthlyRevenue;
- (float)calculateSavingsForTaxes:(float)monthlyRevenue;

@end

Here is it's implementation file:

#import "MMCalculator.h"

#pragma mark - Static Variables
static float const percentageToPayYourself = 0.50f;
static float const percentageToSaveForStuff = 0.20f;
static float const percentageToSaveForProfitFormula = 0.20f;
static float const percentageToSaveForTaxes = 0.10f;

@implementation MMCalculator

#pragma mark - Calculator Methods
- (void)calculateValuesWithMonthlyRevenue:(float)monthlyRevenue
{
    //Call all 4 calculation methods and set calculated properties
    self.calculatedPay = [self calculateYourPay:monthlyRevenue];
    self.calculatedSavingsForStuff = [self calculateSavingsForStuff:monthlyRevenue];
    self.calculatedSavingsForProfitFormula = [self calculateSavingsForProfitFormula:monthlyRevenue];
    self.calculatedSavingsForTaxes = [self calculateSavingsForTaxes:monthlyRevenue];

    //Call delegate method
    [self.delegate mmCalculator:self];
}
- (float)calculateYourPay:(float)monthlyRevenue
{
    return percentageToPayYourself * monthlyRevenue;
}

- (float)calculateSavingsForStuff:(float)monthlyRevenue
{
    return percentageToSaveForStuff * monthlyRevenue;;
}

- (float)calculateSavingsForProfitFormula:(float)monthlyRevenue
{
    return percentageToSaveForProfitFormula * monthlyRevenue;
}

- (float)calculateSavingsForTaxes:(float)monthlyRevenue
{
    return percentageToSaveForTaxes * monthlyRevenue;
}

@end

Here are the key things I am looking for feedback on:

  1. Right now I store the class's calculated values in public properties so they can be accessed by other classes. Should it be the class's responsibility to store these calculated values in it's properties, or should it be the responsibility of the class that is calling this class's methods?.

  2. Did I define my static variables correctly? Are they following best practices?

  3. What do you think of the class's delegate method?

  4. Am I following best practices by using floats to represent money? I have a feeling that I should be using something else.

share|improve this question

3 Answers 3

up vote 3 down vote accepted

Let's address your questions:

  1. I don't think that storing the calculated values in public properties is a good idea, mainly because If I call calculateValuesWithMonthlyRevenue with say, 1000, and then afterwards call calculateYourPay with 400, the other properties don't change to reflect my different monthly revenue. Therefore, I'm not sure why these should be stored as properties at all. I would eliminate calculateValuesWithMonthlyRevenue, and make this class a singleton with only static methods.

  2. In most cases, static constants are written in ALLCAPS or ALL_CAP, depending on your prefered style. I would define these in your header file (if you want these values to be public) or in your implementation file (if you want the values to be private) with #define instead, but that's a matter of personally preference.

  3. You don't show us what you do with this delegate method, so I can't exactly be sure what the point of it is. I would omit it for now.

  4. Using floats is a bit dangerous as it lends itself to annoying floating-point errors such as .3 turning into .299999999. This is really a question of desired precision. If you're fine with being off by a bit, then floats are pretty decent. If not, you will have to define your own type to represent money.

share|improve this answer
    
In regards to point 2, this isn't the recommended convention for Objective-C and #define is heavily recommended against for almost everything in almost every language I'm familiar with. And in this specific case, it would definitely be recommended to use typed constants as he has done (though, they should be named slightly differently). –  nhgrif Aug 19 at 21:57
    
"This is really a question of desired precision." - What would be the best choice if I need pinpoint accuracy since I am dealing with monetary values? –  user3344977 Aug 19 at 22:00
    
@user3344977 Integers. You've marked this answer as accepted, which is fine, but I hope you'll stick around, as I plan on posting quite a few things. –  nhgrif Aug 19 at 22:02
    
I would create a object that has dollars and cents (both are integers) for that. Perhaps you can call it MMMoney? –  mleyfman Aug 19 at 22:04
    
@nhgrif Wait for 10 minutes. I'm posting a new question with some refactoring and things changed. I kind of screwed this one up. I'll link to it in a moment and thanks! –  user3344977 Aug 19 at 22:04

The use of a delegate here is absolutely overkill.

Delegates are useful for plenty of things.

  • When you're performing asynchronous code, using a delegate let's you call back and let the delegate know when the asynchronous code is complete.
  • When you're performing synchronous code but the delegate wants to know about specific events in the code or the delegated object needs to ask the delegate for specific information in a specific order (think about how UITableView's work)
  • When you're performing synchronous code but the delegated object waits for user input, processes it, then informs the delegate that it's done with that.

There are probably other cases, but this covers most typical use cases. Your code doesn't fall into any of these categories.

Your code doesn't fall into any of these categories. And what's worse, we're not actually returning anything useful to the delegate.

In almost every use case, a delegate already has a reference to the object it's delegating. It's not impossible for this to be not be the case, but it's rare. And our code executes synchronously. Any code that you want to put in the delegate's call back method could simply be put in the line right after we call calculateValuesWithMonthlyRevenue:--in the scope in which we already have a reference to the object the delegate method returns.

With that said, it is good for delegate methods to pass a reference to the delegated object when called (self). Nothing prevents Object A from delegating multiple objects that will all call that delegate method. Passing a reference to self is the best way to let the delegate distinguish which object is calling the method.

But here, the delegate pattern is just complete overkill. We're calling very uncomplicated synchronous code that doesn't have various important events that the delegate might want to know about and doesn't ever need to wait for user input.

share|improve this answer
    
Thank you for adding this as well. I was trying way to hard to use a delegate. I have an app with asynchronous code and I'm definitely going to add a delegate to manage that. Your answers always make me learn a lot and you put everything in such an easy to understand way. Thank you! –  user3344977 Aug 19 at 23:22
    
PS. I implemented your changes here: codereview.stackexchange.com/questions/60532/… –  user3344977 Aug 19 at 23:35

The preferred naming style for constants in Objective-C is to prefix the constant with the letter k, and generally after this, follow regular kCamelCasing rules.

For example:

#pragma mark - Const Variables
static CGFloat const kPercentageToPayYourself = 0.50f;
static CGFloat const kPercentageToSaveForStuff = 0.20f;
static CGFloat const kPercentageToSaveForProfitFormula = 0.20f;
static CGFloat const kPercentageToSaveForTaxes = 0.10f;

Notice I renamed the #pragma mark to Const Variables, as I feel this is the more important aspect of these variables.

I've also changed the type to CGFloat. There's not a particularly good reason to use float in Objective-C. A CGFloat is a float on 32-bit systems and a double on 64-bit systems. Meanwhile, a float is always a float and a double is always a double.


- (float)calculateYourPay:(float)monthlyRevenue;
- (float)calculateSavingsForStuff:(float)monthlyRevenue;
- (float)calculateSavingsForProfitFormula:(float)monthlyRevenue;
- (float)calculateSavingsForTaxes:(float)monthlyRevenue;

There's no particularly good reason for these to be instance methods. They don't rely on the state of the instance at all. These should either be class methods or C-Style functions.

If you move them to class methods, the naming is okay. If you change them to C-style functions (which I think I'd prefer to see), we need to fix the naming to something we can be certain won't have any naming collisions.

share|improve this answer
    
Ok great. I'm going to implement most of your changes right now. Thanks for the help. Please see my refactored question here: codereview.stackexchange.com/questions/60527/… –  user3344977 Aug 19 at 22:24
    
+1 for the float advice. I had no idea that CGFloat would dynamically switch like that to accommodate the different systems. –  user3344977 Aug 19 at 22:31

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.