5
\$\begingroup\$

I was writing a bit of code and I was just wondering if I was doing it the right way. I know all the syntax is right and It all works perfectly. I was just wondering if I'm doing it the right way. Should I have another infrastructure, am I following the MVC correctly (there is no V yet, it's mostly just a model). Is this program extensible and enhanceable enough?

It's a very simple program that finds the zeroes of either a first degree polynomial

$$ a x + b = 0 $$

or second degree polynomial.

$$ a x^2 + b x + c = 0 $$

First.h:

#import <Foundation/Foundation.h>

@interface First : NSObject
@property double a, b, x;

- (id) initWithAValue: (double) aValue andBValue: (double) bValue;
- (double) calculate;
@end

First.m:

#import "First.h"

@implementation First
-(id)initWithAValue:(double)aValue andBValue:(double)bValue
{
    self = [super init];
    if (self) {
        self.a = aValue;
        self.b = bValue;

    }
    return self;
}
- (double) calculate {
    self.x = (self.b - 2 * self.b)/self.a;
    return self.x;

}
@end

Second.h:

#import <Foundation/Foundation.h>

@interface Second : NSObject
@property double a,b,c,x1,x2,d;
- (id) initWithAValue: (double) aValue andBValue: (double) bValue andCValue: (double) cValue;
- (double) calculateX1;
- (double) calculateX2;
- (double) calculateD;
@end

Second.m:

#import "Second.h"

@implementation Second
- (id)initWithAValue:(double)aValue andBValue:(double)bValue andCValue:(double)cValue
{
    self = [super init];
    if (self){
        //TODO: check if a = 0
        self.a = aValue;
        self.b = bValue;
        self.c = cValue;
    }
    return self;
}
-(double)calculateD
{
    //TODO: Check if d < 0
    self.d = (self.b * self.b)-(4 * self.a * self.c);
    return self.d;
}
-(double)calculateX1
{
    self.x1 = ((self.b - 2 * self.b) + sqrt([self calculateD]))/ (2 * self.a);
    return self.x1;
}
-(double)calculateX2
{
    self.x2 = ((self.b - 2 * self.b) - sqrt([self calculateD]))/ (2 * self.a);
    return self.x2;
}

@end

Main.m:

#import <Foundation/Foundation.h>
#import "First.h"
#import "Second.h"
int main(int argc, const char * argv[]) {
    @autoreleasepool {
        //This would be replaced by a user interface
        double firstA = 1;
        double firstB = 1;
        double secondA = 1;
        double secondB = 1;
        double secondC = 1;

        First *firstDegree = [[First alloc]initWithAValue:firstA andBValue:firstB];
        NSLog(@"The value of x for your first degree equation is: %.2f",[firstDegree calculate]);

        Second *secondDegree = [[Second alloc]initWithAValue:secondA andBValue:secondB andCValue:secondC];
        NSLog(@"D is: %.2f", [secondDegree calculateD]);
        NSLog(@"The values of x for your second degree equation are: %.2f and: %.2f",[secondDegree calculateX1],[secondDegree calculateX2]);
    }
    return 0;
}
\$\endgroup\$
0

3 Answers 3

2
\$\begingroup\$

The basic notion of encapsulating the mathematical calculations within dedicated model class makes a lot of sense. This sort of code wouldn't be appropriate in a view or controller class. Having said that, a couple of observations:

  1. I would advise against having a property (e.g. x in First or d, x1 and x2 in Second) to hold the result unless you keep track of whether it's been calculated or not. For example, with First, having set a and b, there's no way of knowing when x has been calculated. Furthermore, in your example, you never use x, anyway.

    The simplest answer would be to eliminate these properties for the calculated values altogether, and then just calculate this as needed. Or you could keep the property for the calculated value, but then (a) retire the public calculate method; (b) make the calculated property readonly; and (c) implement your own custom getter that does the calculation.

    If this was an expensive calculation, you could store the calculated value somewhere, but then you'd probably keep track of whether the value had been calculated or not, either returning the previously calculated number or recalculating as necessary (and obviously, if you change any of the coefficients, you'd update the state of the object so that it would know if the roots would need be recalculated). I'm not going to go into that here, as this level of complexity is not called for here, but hopefully you get the idea.

  2. You might consider error handling. The Objective-C pattern would be to take an optional NSError parameter so you can report the error:

    - (double) calculateXInterceptWithError:(NSError * __autoreleasing *)error {
        if (self.a != 0.0) {
            return -self.b/self.a;
        } else {
            if (error) {
                *error = [NSError errorWithDomain:kCalculatorDomain code:kCalculatorErrorDivideByZero userInfo:nil];
            }
            return DBL_MAX;
        }
    }
    

    Whether you think of this a calculating X intercepts or roots or factoring is up to you. But I'd consider a more meaningful name than calculate.

  3. For your roots of the quadratic formula, I'd suggest similar error handling. Furthermore, you might consider handling complex numbers (e.g. use csqrt instead of sqrt, use double complex instead of double, etc.).

  4. In terms of the overall model, you might consider First to be simplified example of Second, thereby combining these two in a single class (e.g., Polynomial or what have you).

    When you start thinking of this as a polynomial solver, then using parameters like a, b, and c might be better captured as an array of coefficients. And the roots might be returned as a single array of value, themselves, rather than having, for example, two different methods of Second, each which returns a different root.

    But, clearly, generalized factoring of a higher order (cubic or greater) polynomial is another kettle of fish, so you probably don't want to go there quite yet.

    Bottom line, given the limited problem you're trying to tackle, all of this may all be overkill. But it is the logical extension of where you appear to be going. Having separate First and Second classes is very limiting and you would often consider more generalized solution.

  5. A couple of tactical observations regarding the provided code snippets:

    • You'd generally define properties to be nonatomic unless you needed atomic (the default) for some reason;

    • You'd generally not reference accessor methods from within init. See Apple's Don’t Use Accessor Methods in Initializer Methods and dealloc. This is one situation (as well as when implementing your own custom accessor methods) that it is prudent to use the ivar. See https://stackoverflow.com/questions/192721/why-shouldnt-i-use-objective-c-2-0-accessors-in-init-dealloc for discussion of rationale. In practice, it's rarely a problem, but, still, it's wise counsel to avoid accessors in init/dealloc.

    • In your custom initializers, nowadays people would use the instancetype return type for clarity rather than id.

    • In the name of the init method, you generally would not include and in the parameter names. Thus, instead of initWithAValue:andBValue:andCValue:, you might have initWithA:b:c:. Clearly, if you generalize the solution, you'd probably retire a, b, and c, anyway, but just an observation of modern method naming conventions.

\$\endgroup\$
2
\$\begingroup\$

As you wrote,

//TODO: check if a = 0

First degree polynomials are just a degenerate case of second degree polynomials, where the leading coefficient is 0. Perhaps you should design your interfaces to reflect that.

I'm not sure that all those calculate… methods are appropriate. Part of the point of object-oriented programming is to give the illusion of "smart data". The object knows when it needs to recalculate; the caller just asks for poly.x and the answer is just there.

In first.m,

- (double) calculate {
    self.x = (self.b - 2 * self.b)/self.a;
    return self.x;

}

could just be

- (double) calculate {
    return self.x = -self.b / self.a;
}

In Objective C, it is not necessary to check if the receiver of a message is nil. Therefore,

if (self) {
    self.a = aValue;
    self.b = bValue;

}

does not need to be in an if block.

\$\endgroup\$
2
  • 2
    \$\begingroup\$ You are generally right about "sending messages to nil", but self = [super init]; if (self) { ... } ; return self; is a common pattern in an init method. On the other hand, setter methods should not be used in an instance method. Compare "Access Instance Variables Directly from Initializer Methods" in "Programming with Objective-C". \$\endgroup\$
    – Martin R
    Oct 25, 2014 at 5:54
  • 1
    \$\begingroup\$ I know the math probably isn't efficient, I was more curious about the infrastructure. And why shouldn't I acces the setter and getters in the init methods? I thought you should avoid directly accessing the ivars \$\endgroup\$
    – SemAllush
    Oct 25, 2014 at 7:52
1
\$\begingroup\$

On a numerical note, the way you calculate the roots of the quadratic leads to unnecessary loss of precision. As test data to show this, you could consider finding the roots of the quadratics

$$ (x-N) (x-\frac{1}{N}) = x^2 - (N+\frac{1}{N})x + 1 $$

for various (largeish) \$N\$ e.g. 1000, 10000, 100000…. You will find you do not get the smaller root \$\frac{1}{N}\$ very accurately, and indeed if you substitute that back into the quadratic the result isn't that close to 0.

The problem arises from the quadratic formula:

$$ r = \frac{-b \pm \sqrt{b^2 - 4ac}}{2a} $$

That's fine in infinite precision arithmetic, but not always in doubles. If, as in the examples I suggested, \$b^2\$ is a lot bigger than \$4ac\$, the square root will be close to \$b\$, and you'll add this to \$-b\$, and lose precision.

A better approach is to compute the root with the bigger magnitude (i.e. if b>0 subtract the square root or if b<=0 add the square root) and compute the other — as long as the root just computed is not 0 — by using the fact that the product of the roots is c.

\$\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.