Design Patterns – Handling Responses in PHP

clean codedesigndesign-patternsobject-orientedPHP

Most of the time when I'm writing some code that handles the response for a certain function call I get the following code structure:

example: This is a function that will handle the authentication for a login system

class Authentication{

function login(){ //This function is called from my Controller
$result=$this->authenticate($username,$password);

if($result=='wrong password'){
   //increase the login trials counter
   //send mail to admin
   //store visitor ip    
}else if($result=='wrong username'){
   //increase the login trials counter
   //do other stuff
}else if($result=='login trials exceeded')
   //do some stuff
}else if($result=='banned ip'){
   //do some stuff
}else if...

function authenticate($username,$password){
   //authenticate the user locally or remotely and return an error code in case a login in fails.
}    
}

Problem

  1. As you can see the code is build on a if/else structure which means a new failure status will mean that I need to add an else if statement which is a violation for the Open Closed Principle.
  2. I get a feeling that the function has different layers of abstraction as I may just increase the login trials counter in one handler, but do more serious stuff in another.
  3. Some of the functions are repeated increase the login trials for example.

I thought about converting the multiple if/else to a factory pattern, but I only used factory to create objects not alter behaviors. Does anyone have a better solution for this?

Note:

This is just an example using a login system. I'm asking for a general solution to this behavior using a well built OO pattern. This kind of if/else handlers appears in too many places in my code and I just used the login system as a simple easy to explain example. My real use cases are to much complicated to post here. 😀

Please don't limit your answer to PHP code and feel free to use the language you prefer.

UPDATE

Another more complicated code example just to clarify my question:

  public function refundAcceptedDisputes() {            
        $this->getRequestedEbayOrdersFromDB(); //get all disputes requested on ebay
        foreach ($this->orders as $order) { /* $order is a Doctrine Entity */
            try {
                if ($this->isDisputeAccepted($order)) { //returns true if dispute was accepted
                    $order->setStatus('accepted');
                    $order->refund(); //refunds the order on ebay and internally in my system
                    $this->insertRecordInOrderHistoryTable($order,'refunded');                        
                } else if ($this->isDisputeCancelled($order)) { //returns true if dispute was cancelled
                    $order->setStatus('cancelled');
                    $this->insertRecordInOrderHistory($order,'cancelled');
                    $order->rollBackRefund(); //cancels the refund on ebay and internally in my system
                } else if ($this->isDisputeOlderThan7Days($order)) { //returns true if 7 days elapsed since the dispute was opened
                    $order->closeDispute(); //closes the dispute on ebay
                    $this->insertRecordInOrderHistoryTable($order,'refunded');
                    $order->refund(); //refunds the order on ebay and internally in my system
                }
            } catch (Exception $e) {
                $order->setStatus('failed');
                $order->setErrorMessage($e->getMessage());
                $this->addLog();//log error
            }
            $order->setUpdatedAt(time());
            $order->save();
        }
    }

function purpose:

  • I am selling games on ebay.
  • If a customers wishes to cancel his order and gets his money back
    (i.e. a Refund) I must open a "Dispute" on ebay first.
  • Once a dispute is opened I must wait for the customer to confirm that
    he agrees to the refund (silly as he's the one who told me to refund,
    but that's how it works on ebay).
  • This functions gets all disputes opened by me and checks their statuses periodically to see if the customer has replied to the dispute or not.
  • The customer may agree (then I refund) or refuse (then I rollback) or may not respond for 7 days (I close the dispute myself then refund).

Best Answer

This is a prime candidate for the Strategy pattern.

For example, this code:

if ($this->isDisputeAccepted($order)) { //returns true if dispute was accepted
    $order->setStatus('accepted');
    $order->refund(); //refunds the order on ebay and internally in my system
    $this->insertRecordInOrderHistoryTable($order,'refunded');                        
} else if ($this->isDisputeCancelled($order)) { //returns true if dispute was cancelled
    $order->setStatus('cancelled');
    $this->insertRecordInOrderHistory($order,'cancelled');
    $order->rollBackRefund(); //cancels the refund on ebay and internally in my system
} else if ($this->isDisputeOlderThan7Days($order)) { //returns true if 7 days elapsed since the dispute was opened
    $order->closeDispute(); //closes the dispute on ebay
    $this->insertRecordInOrderHistoryTable($order,'refunded');
    $order->refund(); //refunds the order on ebay and internally in my system
}

Could be reduced to

var $strategy = $this.getOrderStrategy($order);
$strategy->preProcess();
$strategy->updateOrderHistory($this);
$strategy->postProcess();

where getOrderStrategy wraps the order in a DisputeAcceptedStrategy, DisputeCancelledStrategy, DisputeOlderThan7DaysStrategy, etc. each of which know how to handle the given situation.

Edit, to answer question in comments.

could you please elaborate more on your code. What I understood is that getOrderStrategy is a factory method that returns a strategy object depending on the order status, but what are the preProcess() and preProcess() functions. Also why did you pass $this to updateOrderHistory($this)?

You're focussing on the example, which could be completely inappropriate for your case. I don't have enough detail to be sure of the best implementation, so I came up with a vague example.

The one common piece of code you have is insertRecordInOrderHistoryTable, so I chose to use that (with a slightly more generic name) as the central point of the strategy. I pass $this to it, because it is calling a method on this, with $order and a different string per strategy.

So, basically, I envision each of those looking like this:

public function updateOrderHistory($auth) {
    $auth.insertRecordInOrderHistoryTable($order, 'cancelled');
}

Where $order is a private member of the Strategy (remember I said it should wrap the order) and the second argument is different in each class. Again, this might be entirely inappropriate. You might want to move insertRecordInOrderHistoryTable to a base Strategy class and not pass the Authorization class in. Or you might want to do something entirely different, it was just an example.

Likewise, I limited the rest of the differing code to pre- and postProcess methods. This is almost certainly not the best you can do with it. Give it more appropriate names. Split it down into multiple methods. Whatever makes the calling code more readable.

You might prefer to do this:

var $strategy = $this.getOrderStrategy($order);
$strategy->setStatus();
$strategy->closeDisputeIfNecessary();
$strategy->refundIfNecessary();
$strategy->insertRecordInOrderHistoryTable($this);                        
$strategy->rollBackRefundIfNecessary();

And have some of your Strategies implement empty methods for the "IfNecessary" methods.

Whatever makes the calling code more readable.

Related Topic