3

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?

9
  • possible duplicate of How to tackle a 'branched' arrow head anti-pattern?
    – gnat
    Dec 8, 2014 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 8, 2014 at 14:33
  • 1
    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 8, 2014 at 14:36
  • 2
    @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 8, 2014 at 14:48
  • 1
    I mean both: your query strings, executing the query, catching the exception, formatting the exception log message
    – stijn
    Dec 8, 2014 at 14:57

3 Answers 3

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.

3
  • 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 8, 2014 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 8, 2014 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? Dec 8, 2014 at 15:34
1

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.

1

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.

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.