efficent way to handle the following exception

I have the following code:

public boolean perform(CronJob job)
   final Date lastQueryDateTime = new Date();
   QueryMode queryMode = job.getQueryModeEnum();
   try {
      if (queryMode.getCode().equals("FULL")) 
      {
         calculateCoverage();    

      } else if (queryMode.getCode().equals("INCREMENTAL")) 
      {
          calculateCoverage();
      }        
       job.setLastQueryDate(lastQueryDateTime);
       job.save(job);

       return true;

      } catch (Exception e) {
       LOG.error("Error while calculating the coverage for products.", e);
       return false;
      }
}

private void calculateCoverage()
{

// Some Logic

}

I got in doubt if there is actually too much logic to be handled in between and if the try/catch scope is to wide. Is there a better approach to do this??. I would liket handle the try/catc开发者_运维百科h within calculateCoverage but in that case I will need to pass job as an argurment to calculateCoverage and I think is not that clean.. Any suggestions?

thank you!

It looks like this is a Quartz job (or something like it), which isn't supposed to allow an exception to be thrown.

So it's not that you expect a particular method to throw a particular type of exception (in which case you would want to reduce the scope to that method call). Rather, you want to ensure that if anything inside this method goes awry, you won't put your Cron Job into an error state.

Since the sole purpose of the try/catch in this case is to log any exceptions that are thrown, and you're not planning to handle them in any special way, I think it's perfectly reasonable to have the block encompass the entire contents of the method.

Pay attention to the overall logic, though. Currently an exception in calculating coverage will result in the job not being set to run again. If you want the job to run later even if it fails now, you may want to have a try/finally block in there.

try {
    final Date lastQueryDateTime = new Date();
    QueryMode queryMode = job.getQueryModeEnum();
    String code = queryMode.getCode();
    try{
        if (code.equals("FULL") || code.equals("INCREMENTAL")) 
        {
            calculateCoverage();
        }   
    } finally {
        job.setLastQueryDate(lastQueryDateTime);
        job.save(job);
    }
    return true;
} catch (Exception e) {
    LOG.error("Error while calculating the coverage for products.", e);
    return false;
}

This will still cause any exception in the method to be caught and logged, but if the error happens while calculating coverage, you'll still try to set the job to run again.

While you code is perfectly fine, just for better readability you can write it as follows:

public boolean perform(CronJob job)
   final Date lastQueryDateTime = new Date();
   QueryMode queryMode = job.getQueryModeEnum();
   if (queryMode.getCode().equals("FULL") || queryMode.getCode().equals("INCREMENTAL")) 
   {
         try {
              calculateCoverage();    
         } catch (Exception e) {
            LOG.error("Error while calculating the coverage for products.", e);
           return false;
         }        

   }        
   job.setLastQueryDate(lastQueryDateTime);
   job.save(job);
   return true;
}

private void calculateCoverage() throws Exception
{

// Some Logic

}

I recommend you keep the exception handling outside of the calculateCoverage() method. There's no point in trying to force a design that doesn't make sense so that you can "hide" the exception in your wrapper code.

In fact, seeing as how your perform() method returns false on failure and true on success, I actually recommend just changing the return type of perform() to void, and having the exception thrown up one more level. Throwing exceptions are preferred to returning true or false because they provide a fuller view of what went wrong.

Lastly, if at all possible, it's better to throw/catch specific Exception types (e.g. IllegalStateException) because again, exceptions give the caller with a better idea of what happened.