Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question
2  
You could move the continue statement up into the second try clause, right after the rollback(). Not much of a change, but maybe you like it better? –  Carl Manaster Apr 27 '11 at 17:33
    
@Carl, thanks ;) But the whole thing still looks clumsy to me ;) –  FractalizeR Apr 27 '11 at 19:13
2  
Things can be done, but they are only really helpful if the construction appears repeatedly. Does it? Secondly, if I can't roll a transaction back thats a very serious problem. Serious enough that simply returning seems an inappropriate response. –  Winston Ewert Apr 27 '11 at 23:30

3 Answers 3

up vote 5 down vote accepted

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.

share|improve this answer
    
Can you please demonstrate how will it solve nested try-catch blocks problem? As I understand, I will still need to catch CannotMarkJobCompletedException and try to rollback, if it is caught. And again catch CannotRollbackTransactionException and cancel all operations if caught. –  FractalizeR Apr 27 '11 at 19:12
    
Also SLF4J logger.error() method does not allow to pass single Throwable to it so I still need to provide some error string and pass an exception after. –  FractalizeR Apr 27 '11 at 19:14
    
Oh ;) wait. Did you mean after making distinctive exception classes I should move all logic into a single try-except block and deal with exceptions there? –  FractalizeR Apr 27 '11 at 19:17
    
The main problem is that you need to rollback if marking completed throws an exception, and you need to close silently if rolling back throws an exception. You must either perform these actions in the catch blocks (nested) or alter the control flow in the try blocks (serial). –  David Harkness Apr 28 '11 at 9:00
    
@FractalizeR I suggested you to make your code readable and maintainable by making your exceptions distinct and by serializing your catch blocks instead of nesting them. –  deeperx Apr 28 '11 at 16:20

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.

share|improve this answer
    
Having a method with "CloseSilently" in the name that returns a bool sounds weird to me. Did it close silently? How would you know if it was silent? –  EndangeredMassa Apr 28 '11 at 3:45
    
@EndangeredMassa - rollbackOrCloseSilently() returns true if it rolled back or false if it closed silently, but I agree it's not very intuitive. That's why I prefer the second refactoring. :) –  David Harkness Apr 28 '11 at 8:54

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;
  }
}
share|improve this answer
    
Well, that means abandon some exceptions and move to returning results from method? This is a way, but... well, I wanted to do the same beauty but with exceptions ;) –  FractalizeR Apr 27 '11 at 21:03
    
You're also going to use exceptions, only realization must be by types DbLogicException1, DbLogicException2 etc... Methods unitOfWork.rollback() will throw exception DbLogicException1 and unitOfWork.closeSilently() will throw exception DbLogicException2 (for example) –  siquell Apr 27 '11 at 21:48
    
So, all methods you want will throw exceptions, but catch them in main cycle –  siquell Apr 27 '11 at 21:49

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.