3
\$\begingroup\$

Here is my Model class's new header file:

@interface MMCalculator : NSObject

@property (readonly) CGFloat calculatedPay;
@property (readonly) CGFloat calculatedSavingsForStuff;
@property (readonly) CGFloat calculatedSavingsForProfitFormula;
@property (readonly) CGFloat calculatedSavingsForTaxes;

- (void)calculateValuesWithMonthlyRevenue:(CGFloat)monthlyRevenue;

@end

Here is my Model class's new implementation file:

#import "MMCalculator.h"

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

@interface MMCalculator ()

@property (readwrite) CGFloat calculatedPay;
@property (readwrite) CGFloat calculatedSavingsForStuff;
@property (readwrite) CGFloat calculatedSavingsForProfitFormula;
@property (readwrite) CGFloat calculatedSavingsForTaxes;

@end

@implementation MMCalculator

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

}

+ (CGFloat)calculateYourPay:(CGFloat)monthlyRevenue
{
    return kPercentageToPayYourself * monthlyRevenue;
}

+ (CGFloat)calculateSavingsForStuff:(CGFloat)monthlyRevenue
{
    return kPercentageToSaveForStuff * monthlyRevenue;;
}

+ (CGFloat)calculateSavingsForProfitFormula:(CGFloat)monthlyRevenue
{
    return kPercentageToSaveForProfitFormula * monthlyRevenue;
}

+ (CGFloat)calculateSavingsForTaxes:(CGFloat)monthlyRevenue
{
    return kPercentageToSaveForTaxes * monthlyRevenue;
}

@end

Key things I am looking for advice on:

  1. Are my constants defined in the correct location in my Model class's implementation file?

  2. Even though the compiler does not force me to, would it be better to declare my private methods in the class extension? I want to make sure I'm following best practices.

  3. Now that the Model's instance methods have been changed to class methods, should I change the public calculateValuesWithMonthlyRevenue: method to a class method as well?

  4. Any other general comments are welcome.

\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

Are my constants defined in the correct location in my Model class's implementation file?

It is impossible to tell. Where a variable needs to be declared is entirely dependent upon its intended scope.

  • If a variable is used only within a single block (within an if, else, or loop, or anything else of this nature), it should be declared within that block.
  • If a variable is used only in a single function, it should be declared in that function.
  • If a variable is used in multiple functions in a file, it should be declared in the .m file so it can be seen throughout the file, but not within other files in the project.
  • If a variable is used in multiple files, it should be declared in a .h file so that it may be imported across the multiple files.

Even though the compiler does not force me to, would it be better to declare my private methods in the class extension? I want to make sure I'm following best practices.

There's no particular need to declaring methods in class extensions in Objective-C. I don't know of any real benefit to doing so.


Now that the Model's instance methods have been changed to class methods, should I change the public calculateValuesWithMonthlyRevenue: method to a class method as well?

This one can't be changed, at least not quite as easily. You can't refer to a class's instance variables within a class method.

Perhaps what you should do is instead change it to more of a factory method that might look a bit like this:

+ (instancetype)calculatedValuesForMonthlyRevenue:(CGFloat)monthlyRevenue {
    return [[self alloc] initWithValuesForMonthlyRevenue:monthlyRevenue];
}

- (instancetype)initWithValuesForMonthlyRevenue:(CGFloat)monthlyRevenue {
    self = [super init];

    if (self) {
        _calculatedPay = [MMCalculator calculateYourPay:monthlyRevenue];
        _calculatedSavingsForStuff = [MMCalculator calculateSavingsForStuff:monthlyRevenue];
        _calculatedSavingsForProfitFormula = [MMCalculator calculateSavingsForProfitFormula:monthlyRevenue];
        _calculatedSavingsForTaxes = [MMCalculator calculateSavingsForTaxes:monthlyRevenue];
    }

    return self;
}

Any other general comments are welcome.

The class name, MMCalculator needs some work. This does a more specific job then a regular calculator. This is more like a personal finance calculator if anything.


Any other general comments are welcome.

You really shouldn't represent money with floating point numbers.

Instead, you should probably create a class for holding money.

Money.h

@interface Money

@property (readonly) int dollars;
@property (readonly) int cents;

+ (instancetype)moneyWithDollars:(int)dollars cents:(int)cents;
- (instancetype)initWithDollars:(int)dollars cents:(int)cents;

@end

Money.m

@implementation Money {
    long long _totalCents;
}

- (int)dollars {
    return _totalCents/100;
}

- (int)cents {
    return _totalCents%100;
}

+ (instancetype)moneyWithDollars:(int)dollars cents:(int)cents {
    return [[self alloc] initWithDollars:dollars cents:cents];
}

- (instancetype)initWithDollars:(int)dollars cents:(int)cents {
    self = [super init];

    if (self) {
        _totalCents = cents + (100 * dollars);
    } 

    return self;
}

@end

The class could be rounded out with various math methods as well.

\$\endgroup\$
3
  • \$\begingroup\$ I like your factory method idea. That will be much cleaner and make more sense, and I will definitely create a new class to represent the money as well. I need pinpoint accuracy. \$\endgroup\$ Commented Aug 20, 2014 at 0:11
  • \$\begingroup\$ I want to make sure I fully understand your comments on my constants. Because each constant is only used inside a single, specific class method, I should declare and define each constant inside the method that it is being used in? \$\endgroup\$ Commented Aug 20, 2014 at 0:14
  • \$\begingroup\$ Correct. Unless you have any intention of using that constant else where. Variables (including constants) should always be declared in the tightest scope possible. \$\endgroup\$
    – nhgrif
    Commented Aug 20, 2014 at 0:15
1
\$\begingroup\$

Definitely pay attention to nhgrif's recommendations, he knows what he is talking about. I would like to talk about something that you might or might not be able to change, so please take it with a grain of salt.

I see this:

@property (readonly) CGFloat calculatedPay;
@property (readonly) CGFloat calculatedSavingsForStuff;
@property (readonly) CGFloat calculatedSavingsForProfitFormula;
@property (readonly) CGFloat calculatedSavingsForTaxes;

And this:

static CGFloat const kPercentageToPayYourself = 0.50;
static CGFloat const kPercentageToSaveForStuff = 0.20;
static CGFloat const kPercentageToSaveForProfitFormula = 0.20;
static CGFloat const kPercentageToSaveForTaxes = 0.10;

And finally this:

_calculatedPay = [MMCalculator calculateYourPay:monthlyRevenue];
_calculatedSavingsForStuff = [MMCalculator calculateSavingsForStuff:monthlyRevenue];
_calculatedSavingsForProfitFormula = [MMCalculator calculateSavingsForProfitFormula:monthlyRevenue];
_calculatedSavingsForTaxes = [MMCalculator calculateSavingsForTaxes:monthlyRevenue];

What I do not understand is why you need to set things up in this inflexible way? It seems to me like it would make more sense and also be more extensible if you could create these calculations on the fly. I assume this is just a program for personal use, but what if you decide later that you want to calculate these values in another way? You will have to go back to the code and add more properties or alter the const values that you are declaring, and recompile the code when you are done.

I would expect to see something more like this:

-(void) calculateSavingsForPay:(int)monthlyPay percentToSave:(float)percentToSave savingsReason:(NSString *)savingsReason

With a method like this, you could calculate these values on the fly, for whatever percentages you want and with whatever name that you want. You could still save the percentages that you want as consts, or you could load that information from another source such that you may not need to recompile the calculator if you want to make changes. I have recently begun using property lists to achieve this.

One step further would be to save these values together as a class, and pass an object of that class into the calculation method or class, and return the result in whatever format that you need.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.