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.

Would you be kind to make me a code review? It would be nice to have some comments, whether you would have done something differently and so on. I am not sure about the way I handle the exception. Is there a better way?

@Transactional
public classServiceImpl implements Service {

    private static final Logger LOG = Logger.getLogger(HermesServiceImpl.class);


    @Autowired(required = true)
    private Client client;

    @Autowired
    private SubmissionDAO submissionDAO;

    @Autowired
    private SubmissionSender hrsQueueSender;

    @Transactional
    @Override
    public void registerDocument(String submissionId) {

        final long start = System.currentTimeMillis();
        final List<HermesItem> items = submissionDAO.getSubmissionItems(submissionId);
        RegistrationSummary regSummary = null;

        if (!items.isEmpty()) {
            final HrsDocumentBuilder builder = new HrsDocumentBuilder();
            final Date docDate = new Date();
            builder.setTitle(submissionId);
            builder.setDocumentDate(docDate);
            builder.setSentDate(docDate);
            builder.setMailType(HermesMailType.INTERNAL);

            /* Document is constructed, try to invoke Hermes - upload annexes and register document. */
            try {
                regSummary = client.registerDocument(builder.buildDocument());
            } catch (final ClientException e) {
                LOG.error("Unable to register document with Submission Id= [" + submissionId + "], " + e.getMessage());
            }
            if (regSummary != null) {
                final String hermesDocId = regSummary.getHermesDocumentId();
                /* Update the DB with the documentID received from Hermes. */
                final int update = submissionDAO.updateSubmission(submissionId, hermesDocId, SubmissionStatus.HRS_REGISTERED.name());

                if (update < 1) {
                LOG.error("update error");
                }
            } else {
                submissionDAO.updateSubmission(submissionId, null, SubmissionStatus.HRS_FAILED.name());
                hrsQueueSender.sendMesage(submissionId);
            }

        } else {
            LOG.error("Not found");
            submissionDAO.updateSubmission(submissionId, null, SubmissionStatus.ANNEXES_NOT_FOUND.name());
        }
    }
}
share|improve this question
add comment

1 Answer

up vote 0 down vote accepted

Few points:

  • Try to avoid long or nested if/else
  • Sub-divide tasks in smaller functions(clean and more readable code + easier unit testing)
  • Minimize variable scope(try to declare variables in the scope where you really need them)
  • Think about Exception in context of your business domain, may be using builder pattern (like HrsDocumentBuilder) can be useful to have consistent logging message for same type of exception(override toString() method).

We can refactor the method registerDocument() like this:

You may replace

final long start = System.currentTimeMillis();

with StopWatch

@Transactional
@Override
public void registerDocument(String submissionId) {
final long start = System.currentTimeMillis();    
final List<HermesItem> items = submissionDAO.getSubmissionItems(submissionId);

if (!hasItems(items)) //extract else to method
    return;

final HrsDocumentBuilder builder = ...builder.setMailType(HermesMailType.INTERNAL);//stays the same
try {
      // minimized the scope of local variable, you need regSummary in try block so declare it here.
        RegistrationSummary regSummary = client.registerDocument(builder.buildDocument()); // will rename buildDocument() -> build() 
        update(regSummary);

} catch (final ClientException e) { 

       //To standarized log message we can refactor it like :
        ClientExceptionBuilder b = new ClientExceptionBuilder(e).submissionId(submissionId).build();// override toString()
        LOG.error(b.getMessage());
}
}//function registerDocument() end here

Since all other method can be easily extracted from else blocks, i am posting only the update(regSummary) because this function can be further subdivide into two smaller functions like this:

void update(regSummary) {

    if(!hasSummary()) //extract else to method 
        return 

    final String hermesDocId = regSummary.getHermesDocumentId();
    /* Update the DB with the documentID received from Hermes. */
    final int update = submissionDAO.updateSubmission(submissionId, hermesDocId, SubmissionStatus.HRS_REGISTERED.name());

    if (update < 1) 
        LOG.error("update error");

}
share|improve this answer
add comment

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.