Making a job completed, rolling back on error

Question

I have the following construction in the program:

  while(true) {
    <...>
    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();
            return;
        }
        continue;
    }
    <...>
  }

So, the logic is the following:

1. Trying to mark the job completed
2. If we fail, try to rollback transaction and proceed to next job item
3. If transaction rollback failed, something is seriously wrong here, aborting everything

This construction looks a little clumsy to me. Is there a way to refactor it so it would become... beautiful?

Answer 1

Yes.

1. Throw specialized exceptions like CannotMarkJobCompletedException and CannotRollbackTransactionException...
2. Make your toString() method on exceptions to return something you want what your logger to log
3. Instead of doing: ...CAN_..._COMPLETED + jobId, you may do: thrown exception already knows this jobId
4. Store the thrown exception in a variable and call your logger once

By doing all of that you will solve the problem of having nested try-catch blocks.

Answer 2

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 returns when a unit of work cannot be rolled back makes me suspect that the method should actually throw an exception in this case.

Answer 3

How about this:

while(true) {
  <...>
  try {
    JobResult const result = JobManager.markJobCompleted(unitOfWork.getSqlFactory(), jobId, dataOut);
    if (JobResult::Fail == result && unitOfWork.rollback())
    {
      unitOfWork.closeSilently();
    }
  }
  catch (DbLogicException1 e) {
    logger.error(JobManager.CANNOT_MARK_JOB_COMPLETED + jobId, e);
    continue;
  }
  catch (DbLogicException2 e) {
    logger.error(UnitOfWork.CANNOT_ROLLBACK_TRANSACTION, e1);
    return;
  }
}