Class design - should methods call other methods?

Question

I'm writing a very basic custom class for coupons and I've come up with a basic layout for the class, which consists of a number of small methods as I generally understand is a best practice.

What I'm unsure of is how the flow of a class should be dictated. Right now the code looks something like this:

public function isCouponValid($coupon) {
    if (strlen($coupon) != 8) return false;

    $query = $this->db->prepare('SELECT coupon_code FROM coupons WHERE coupon_code=:coupon');
    try ($query->execute(array(':coupon' => $coupon))) {
        while ($row = $query->fetch()) {
            return true;
        }   
    } catch (PDOException $e) {
        $log->addError('Could not execute SQL query: '.$query.' '.$e->getMessage());
    }

    return false;
}

public function isCouponUsed($coupon) {
    if (self::isCouponValid($coupon)) {
        $query = $this->db->prepare('SELECT coupon_used FROM coupons WHERE coupon_code=:coupon');
        try ($query->execute(array(':coupon' => $coupon))) {
            while ($row = $query->fetch()) {
                return ($row['coupon_used'] == '1') ? true : false;
            }   
        } catch (PDOException $e) {
            $log->addError('Could not execute SQL query: '.$query.' '.$e->getMessage());
        }
    }

    return false;
}

public function setCouponUsed($coupon) {
    if (self::isCouponValid($coupon) && !self::isCouponUsed($coupon)) {
        etc...
    }
}

Basically I have these checks integrated into the class right now - isCouponUsed() calls isCouponValid() in order to check that it's valid first. Is this a good idea, or is it better for me to use isCouponValid() outside of the class and then pass that value to isCouponUsed(), such that isCouponUsed() should just be concerned with validating a coupon and not check if that code is valid first?

MichelHenrich · Accepted Answer · 2014-12-09T02:12:10.983

I think this comes down to encapsulation.

If you have to offer public methods to check Coupons, then you can't guarantee that the user of your API will call isCouponValid before calling isCouponUsed, so you have to add the extra check. But these situations are always questionable, and usually suggests that your design does not follow principles such as TellDon'tAsk.

But - and I believe this is the case here - if the validations are part of the behavior of an object, such as the setCoupon method seems to suggest, then you can encapsulate them as private methods (implementation details) of the object. As soon as they are encapsulated, the object is in full control of the order of execution, so you can choose to only call isCouponUsed when the coupon is indeed valid, such as already implemented in the if statement of the setCoupon method.

score 1 · Answer 2 · answered Dec 08 '14 at 15:32

You should be concerned about classes driving the behaviour of other classes. This means to be wary(not necessarily always avoiding) the dot operator.

Suppose you are a pizza delivery guy, and you come to my house looking to deliver me a pizza, are you going to reach into my wallet and take my money? What if my money is on the fridge? This is called high coupling.

It means that in order to program additional functionality the PizzaGuy class always has to know about the implementation of the PizzaOrderer. It is a better idea for the PizzaGuy object to simply ask the PizzaOrderer for some money. The pizza guy can then be reused for different instances of the PizzaOrder class such as:

SearchesCouchForMoneyPizzaOrderer 
PayingWithDebitForATenDollarOrderGuy

...

This is called the law of demeter, and is an important rule for reducing coupling, allowing for reuse of your code.

score 1 · Answer 3 · answered Dec 09 '14 at 15:30

Basically I have these checks integrated into the class right now - isCouponUsed() calls isCouponValid() in order to check that it's valid first. Is this a good idea, or is it better for me to use isCouponValid() outside of the class and then pass that value to isCouponUsed(), such that isCouponUsed() should just be concerned with validating a coupon and not check if that code is valid first?

A method calling other methods to perform its job is fine and fits well into object orientated design. Think of objects as people that perform a behavior in the system. The less an individual person has to do the smoother the system will work.

Imagine you ring a call center to order a new phone. You talk to a customer facing "object", but that "object" might query a whole host of other objects internal to the systems in order to carry out the behavior of "provide customer with new phone". As an external customer you don't see or care about any of that, but the call centre would fall a part if the guy on the phone also had to check inventory, run to the warehouse to fetch the phone, parcel it, post it etc.

Likewise if a coupon needs to first check if it is valid before checking if it is used that is fine. The external object that sent the message "are you valid" to the coupon object doesn't care what it does in order to perform this action, but separating out responsibility to a number of smaller object with clear behavior is good design (in the same way that the guy who answers the phone in the call centre doesn't perform every action involved in getting you a new phone, that would be silly)

Your code might make more sense if you moved the methods into the coupon object. Why can't the coupon validate itself, and track if it is used itself. You will probably greatly reduce the number of public methods on the object.

Class design - should methods call other methods?

3 Answers3