I made a quick refactoring to extract a new method UnitOfWork::rollbackOrCloseSilently()
, but I'm not sure it really helps.
public boolean rollbackOrCloseSilently() {
try {
rollback();
return true;
}
catch (DbLogicException e) {
logger.error(CANNOT_ROLLBACK_TRANSACTION, e);
closeSilently();
return false;
}
}
While it merely hides the complexity in another method, it does clean up your original loop a little:
while(true) {
<...>
try {
JobManager.markJobCompleted(unitOfWork.getSqlFactory(), jobId, dataOut);
} catch (DbLogicException e) {
logger.error(JobManager.CANNOT_MARK_JOB_COMPLETED + jobId, e);
if (!unitOfWork.rollbackOrCloseSilently()) {
return;
}
continue;
}
<...>
}
The problem is that it now hides the logging inside a method that doesn't seem like it should log, and the method returns a success status which is less desirable to exceptions in my view.
Starting over, another simple improvement would be to extract the full code between pairs of <...>
into a new method of the same class. Again you have three possible exits: continue processing normally if the job is marked completed okay, continue the loop early if it fails to be marked but rolls back, or exit the method entirely if it cannot be rolled back. Here's where specific exceptions as İnanç Gümüş recommended could help:
while (true) {
<...>
try {
markJobCompletedOrRollback(unitOfWork, jobId, dataOut);
}
catch (MarkCompletedException e) {
continue;
}
catch (RollbackException e) {
return;
}
<...>
}
...
private markJobCompletedOrRollback(UnitOfWork unitOfWork, ? jobId, ? dataOut) {
try {
JobManager.markJobCompleted(unitOfWork.getSqlFactory(), jobId, dataOut);
}
catch (DbLogicException e) {
logger.error(JobManager.CANNOT_MARK_JOB_COMPLETED + jobId, e);
try {
unitOfWork.rollback();
}
catch (DbLogicException e1) {
logger.error(UnitOfWork.CANNOT_ROLLBACK_TRANSACTION, e1);
unitOfWork.closeSilently();
throw new RollbackException();
}
throw new MarkCompletedException();
}
}
While it doesn't address the nested try-catch blocks, it does move them out of the original loop which may improve that method's readability. At least now this new method does one logical thing: mark the job completed or roll it back.
That your original code return
s when a unit of work cannot be rolled back makes me suspect that the method should actually throw an exception in this case.
continue
statement up into the secondtry
clause, right after therollback()
. Not much of a change, but maybe you like it better? \$\endgroup\$ – Carl Manaster Apr 27 '11 at 17:33