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.
There's a lot to say here.
Inventory
's api looks like a value object (since it has no identity, on its own, I suppose that it belongs to a Player
). If so, it should be immutable, returning a new instance each time you add or remove an Item
.
- Your
Item
class looks like a shared identifier, thus it should override equals
and hashcode
and (strictly speaking, in DDD term) it should not hold any reference to the icon
since it's not useful for its own invariants. You should use a domain service that take the Item and provide the proper icon instead (probably wrapping a statically defined Hashtable). However, be pragmatic with this: if you measure actual performance issues with such (formally correct) approach, keep the field there.
- The
Slot
, as it stands, cannot be considered a domain object, because it does not implement equality in a domain relevant way (it ignores the quantity). This is perfecly correct for a private class internally used by the Inventory
, but should not be exposed to clients (as in the second Inventory
's implementation of the first solution you proposed). This because the clients could compare for equality two Slot
s from two different players, obtaining unreliable results.
- To get rid of most boilerplate code, I would code
- a supporting functor interface like
ItemQuantityChanger
(below)
- a "functional" immutable collection, say
ItemsSet
, with a metod of signature ItemsSet set(Item item, ItemQuantityChanger transformation)
that iterates over a simple array for the item to change. Such method would always create a new ItemsSet to return, without the Item (if the trasformation returns zero), with a new Item if the item is not found (with the quantity returned by applying the transformation to zero) or simply with the previous item coupled with the result of the trasformation.
However, concretely, I would just use an ImmutableList.Builder
, since the functional design described in 4 adds few value in this case. A single if, in a method, is not that huge smell.
public interface ItemQuantityChanger {
/**
* Accept the current quantity of an Item and return the new one.
*/
int applyTo(int currentQuantity);
}
Best Answer
Though probably not recognised as a design pattern, what you describe would be called a Mapper.
It translates one object into another.
Implementations can be found at AutoMapper.org and at CodeProject.