This is a piece of code that I was wondering if it should be refactored to make it more complied with the Clean Code practices.
This is a class which is responsible for refunding some orders made by customers.
class RefundServiceInvoker {
private $_orders;
public function refundOrder() {
$this->getOrdersFromDB(); //This function gets all orders from DB and sets $_orders
foreach ($this->_orders as $order) {
try {
$order->refund(); //Some lines may throw an exception when refunded due to some business logic (ex. the order was already shipped)
$this->updateOrderStatus('refunded')
} catch (Exception $e) {
$this->logError($e);
$this->sendMailToAdmin();
}
}
}
}
of course this code is highly simplified than my original code.
My main problem is if $order->refund(); throws an exception it will be caught and logged to DB then a mail is sent. However what if $this->logError($e); itself throws an exception? or what if the mail server was down and an exception was thrown?
Better what if the DB was down itself and $this->getOrdersFromDB(); throws an exception?
My first solution was to wrap everything in one big try{}catch{}:
public function refundOrder() {
try {
$this->getOrdersFromDB(); //This function gets all orders from DB and sets $_orders
foreach ($this->_orders as $order) {
$order->refund(); //Some lines may throw an exception when refunded due to some business logic (ex. the order was already shipped)
$this->updateOrderStatus('refunded')
} catch (Exception $e) {
$this->logError($e);
$this->sendMailToAdmin();
}
}
}
But that would mean if one order fails then all fails!! Should I put 2 try{}catch{} one for the whole function and the other for each order? But in this case too the functions in the catch might throw an exception that won't be caught.
Note:
The application is built using Zend framework 1.11.11.
Thanks in advance.
If you need to log and mail every exception, then you need something like this:
class RefundServiceInvoker {
private $_orders;
public function refundOrder() {
try {
$this->getOrdersFromDB();
foreach ($this->_orders as $order) {
try {
$order->refund();
} catch (MyBusinessException $e) {
# deals with the problematic $order without stopping the loop
$this->logError($e);
$this->sendMailToAdmin();
}
$this->updateOrderStatus('refunded');
}
} catch(Exception $e) {
# deals with unexpected bugs
$this->logError($e);
$this->sendMailToAdmin();
}
}
}
But you have to put a try/catch inside log and mail methods to prevent them to throw any exception when servers are offline, for example. If you don’t, the $orders loop will be stopped when log/mail fails.
If you need to log/mail only your business exception in refund() method, then you need something like this:
class RefundServiceInvoker {
private $_orders;
public function refundOrder() {
$this->getOrdersFromDB();
foreach ($this->_orders as $order) {
try {
$order->refund();
} catch (MyBusinessException $e) {
# deals with the problematic $order without stopping the loop
$this->logError($e);
$this->sendMailToAdmin();
}
$this->updateOrderStatus('refunded');
}
}
}
Any other exception will lead to an http 500 – internal server error page, which is usually what you want, because it’s an unexpected bug.
There’re other ways to handle this, but as you see, it depends on your needs.
There is no magic bullet to solve such issues. If a function can throw and you care about it, you have to wrap try/catch around it -- simple as that.
To move into specifics: it's not really possible to evaluate the merits of this or that approach without having much more information about the architecture of your app, but here are some general suggestions:
refundOrder. Make sure that orders have been loaded successfully; make sure the orders you are going to operate on are all refundable (what's the purpose of trying to refund an order that is not refundable due to business logic? shouldn't the user/operator be notified of this before a refund is attempted?).try/catch, but perhaps not all inside refundOrder. The outer block is meant to catch any errors that were really unexpected, so what's the point in catching them inside refundOrder only? Don't you want to do that in other parts of you app as well? The innermost try/catch is necessary so that one non-refundable order won't kill all the process.As logError($e) and sendMailToAdmin() are probably functions that are used in last resort, typically in try/catch blocks, I would ensure that they never throw an Exception.
function logError($e)
{
try
{
//your logic that may throw an Exception here
}
catch {}
}
function sendMailToAdmin($e)
{
try
{
//your logic that may throw an Exception here
}
catch {}
}