Design pattern for handling a response

Question

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. :D

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).

Answer 1

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.

Answer 2

The strategy pattern is a good suggestion if you truly want to decentralize your logic, but it seems like indirection overkill for examples as small as yours. Personally, I would employ the "write smaller functions" pattern, like:

if($result=='wrong password')
   wrongPassword();
else if($result=='wrong username')
   wrongUsername();
else if($result=='login trials exceeded')
   excessiveTries();
else if($result=='banned ip')
   bannedIp();

Answer 3

When you start having a bunch of if/then/else statements to handle a status, consider the State Pattern.

There was a question on a particular way of using it: Does this implementation of the state pattern make sense?

I'm new to this patter, but I put up the answer anyway to make sure I understand when to use it (Avoid the "all problems look like nails to a hammer.").

Answer 4

As I said in my comments, complex logic doesn't really change anything.

You want to process a disputed order. There are multiple ways of doing that. Disputed order type can be Enum:

public void ProcessDisputedOrder(DisputedOrder order)
{
   switch (order.Type)
   {
       case DisputedOrderType.Canceled:
          var strategy = new StrategyForDisputedCanceledOrder();
          strategy.Process(order);  
          break;

       case DisputedOrderType.LessThan7Days:
          var strategy = new DifferentStrategy();
          strategy.Process(order);
          break;

       default: 
          throw new NotImplementedException();
   }
}

There are many ways of doing this. You can have inheritance hierarchy of Order, DisputedOrder, DisputedOrderLessThan7Days, DisputedOrderCanceled, etc. This is not nice, but it would also work.

In my example above I look at order type and get relevant strategy for that. You could encapsulate that process into a factory:

var strategy = DisputedOrderStrategyFactory.Instance.Build(order.Type);

This would look at the order type and give you a correct strategy for that type of order.

You might end up with something on the lines of:

public void ProcessDisputedOrder(DisputedOrder order)
{
   var strategy = DisputedOrderStrategyFactory.Instance.Build(order.Type);   
   strategy.Process(order);
}

Original Answer, no longer relevant as I thought you were after something simpler:

I see the following concerns here:

• Check for banned IP. Checks whether user's IP is in a banned IP range. You will execute this no matter what.
• Check if trials exceeded. Checks whether user have exceeded their login attempts. You will execute this no matter what.
• Authenticate user. Attempt to authenticate user.

I would do the following:

CheckBannedIP(login.IP);
CheckLoginTrial(login);

Authenticate(login.Username, login.Password);

public void CheckBannedIP(string ip)
{
    // If banned then re-direct, else do nothing.
}

public void CheckLoginTrial(LoginAttempt login)
{
    // If exceeded trials, then inform user, else do nothing
}

public void Authenticate(string username, string password)
{
     // Attempt to authenticate. On success redirect, else catch any errors and inform the user. 
}

Currently your example has too many responsibilities. All I did, was encapsulate those responsibilities within methods. Code looks cleaner and you don't have condition statements all over the place.

Factory encapsulates construction of objects. You don't need to encapsulate construction of anything in your example, all you need to do is to separate your concerns.