I've written this in C, so I can't guarantee that it'll cut-and-paste into Java.
If you're certain you want to retry DoA only once, use can use one line:
bool DoIt()
{
return DoA() || DoB() || DoA();
}
bool DoA() { } // Return true if successful.
bool DoB() { } // Return true if successful.
If you want the option of more than one retry, use this loop.
bool DoIt(int retryLimit)
{
if (DoA()) return true;
for (int retries = 0; retries < retryLimit; retries++)
{
if (DoB()) return true;
if (DoA()) return true;
}
return false;
}
Edit: If you really cannot make time to extract DoA
and DoB
, then try the following block. Remember that comments are your friend:
bool retry = false;
bool success = false;
do
{
// Do A.
success = // true or false
if (success) break;
// Attempt the above code twice and the below code once.
if (retrying) break;
retrying = true;
// Do B.
success = // true or false
if (success) break;
}
if (success)
{
// Celebrate.
}
else
{
// Commiserate.
}
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.
Best Answer
What language are you using? Depending on the language(synchronious, asynchronious) you will have different solutions.
If it is JavaScript, promise is indeed the way to go. Promises in JavaScript
If it is something like C#, you probably don't want to have a loop like you indicated, because you will be blocking the thread. In that scenario, I would look into setting up queuing system in combination with Command Design Pattern:
rabbitMQ | 0mq
Using queue you can send failed messages into the retry queue and retry them in the order they were submitted, potentially with some delay.