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?

possible duplicate of [How to tackle a 'branched' arrow head anti-pattern?](http://programmers.stackexchange.com/questions/205803/how-to-tackle-a-branched-arrow-head-anti-pattern) — gnat, Dec 08 '14 at 14:29
@gnat I don't believe this is a duplicate, my question is not so much about branching but whether data should be validated in each method or whether methods should assume the input is already validated when fed in — Roy, Dec 08 '14 at 14:33
did you check duplicate target questions linked to that one? [Style for control flow with validation checks](http://programmers.stackexchange.com/questions/148849/style-for-control-flow-with-validation-checks) and [Approaches to checking multiple conditions?](http://programmers.stackexchange.com/questions/191208/approaches-to-checking-multiple-conditions) — gnat, Dec 08 '14 at 14:35
Not really related to your question, but one thing this code could really benefit from is minimizing the amount of repetition aka not using copy/past coding aka adhering to the DRY pinciple: there are about 5 lines in each function which are *exactly* the same. That just screams for refactoring into one single method. On which then you can apply the question again :] — stijn, Dec 08 '14 at 14:36
@stijn Do you mean the exception handling or running SQL queries generally? — Roy, Dec 08 '14 at 14:45
@gnat Style for control flow is a better match than the currently linked duplicate IMO, though in my case I can't lump all validation inside one method as my program has steps to follow where not all data will be initially present — Roy, Dec 08 '14 at 14:48
I mean both: your query strings, executing the query, catching the exception, formatting the exception log message — stijn, Dec 08 '14 at 14:57
@stijn I didn't bother to abstract the DB since this is a relatively simple and short-lived script but abstracting the exception handling is something I would be very interested in, do you have any recommended reading on the matter? Thanks :) — Roy, Dec 08 '14 at 15:23
It's not really 'abstracting' the DB, that's one step further, I rather meant things even as simple as using a string constant instead of blatantly copy/pasting the complete query string so that when you have to change the string you have to do it in one place only (though in this case I'd just make a single function which executes the query and allows iteration over all resulting elements or so) - for the exeption handling: I don't really have sources for that, I also don't know php at all :] — stijn, Dec 08 '14 at 15:56

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

3

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.

edited Dec 09 '14 at 02:12

answered Dec 08 '14 at 15:06

MichelHenrich

6,225
1
27
29

For what it's worth, this is meant to be a simple script that will only be used by me and won't be used for very long. So my understanding is that in this situation, since I control the flow, I can forgo the checks knowing I'll implement them properly as and when they're called? – Roy Dec 08 '14 at 15:09
1

@Roy The simple answer is to do what works for you. If the time you'd have to take to revise it isn't worth it for a temporary script then don't do it. YAGNI. – ravibhagw Dec 08 '14 at 15:12
@Roy only you can make that decision, like baultista says, you need to consider the scope. One thing to think of though, is will you be able to recall all the nuances 3 days from now? 3 months? – davidahines Dec 08 '14 at 15:34

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