I know several ways to solve it (probably those that are the most obvious ones) but I'm hoping others can see a more elegant / less hacky solution.
(Perhaps there's a really embarrassingly obvious answer but after several interrupted nights of sleep due to our newborn I'm just not seeing it)
The issue at hand is we want to perform a business operation and keep track (in memory) of the job's state. It's not critical that the job succeed but it is ABSOLUTELY critical that we record the state of the Job correctly.
So first off here is some pseudocode:
Set Job Status to "New"
Start Job
If job fails set status to "Failed"
If job succeeds set status to "Success"
Simple right? Well the problem comes in when you have exception handling so here's a quick first cut of the code that I wrote
Version #1
Clearly I'd want to store / log why things failed but let's not worry about that for the moment.
Anyway so that's pretty simple, and looks correct, but wait - what if an unchecked exception is thrown. I really don't want to catch RuntimeException (let's not even thing about catching an Error / Throwable) .
So perhaps I add a finally block - so that if the status is still "NEW" then clearly an exception was thrown that was not the Checked exception.
So here's Version #2
But that looks so hacky and also someone reading the code is really going to have to think about it to see if all paths are appropriate (as compared to the pseudocode which is "obviously" right).
True I could mimic the pseudocode outlined above and catch any exceptions in the launchJob() method (including RuntimeException?) to get code as follows:
Version #3
but perhaps as the caller of launchJob() I am not aware of the catching of all those Exceptions (including Runtime?) in the method (say it's some vendor / 3rd party / external API) and so I go back to wrapping again but this time without the catch block
Version #4
A little cleaner but still - the pseudocode is the cleanest of all - why can't I map it to Java more easily and not worry about possible Exceptions and the resulting ugly code?
Thoughts? Suggestions?
(p.s. this problem highlights for me one of the key issues developers must face that isn't talked about much - not just solving the abstract problem - for me the solution to that is the pseudocode - but here the hard issue is mapping the solution to a language and understanding the behavior of that language - exception handling, how unchecked exception work etc.)
10 comments:
Why not just catch and rethrow RuntimeException? You catching it won't hurt it.
I like the first version with a minor change: Extract the first statement (setJobStatus) and put it above the try. If needed add another try above the moved statement and a catch just after it, in case you need some error handling for the quite unusual issue that setJobStatus fails.
My preference would be to separate the two concepts of tracking the program flow with tracking the status of the job.
I.e. introduce a new boolean, and keep the final state-changing code in the same block:
boolean successfulTermination = false;
try {
setJobStatus(jobID, "NEW");
launchJob(job);
successfulTermination= true;
}
finally
{
if (successfulTermination)
setJobStatus(jobID, "SUCCESS");
else
setJobStatus(jobID, "FAILED");
}
As a side effect, this is likely to be faster (I'm guessing getJobStatus() hides a hashmap lookup, and it removes the object equality check).
But in terms of design there's not much to argue either way.
The compiler will add a catch-and-rethrow for your finally block so it wouldn't be that terrible to make it explicit - although be sure to consider adding one for Error as well as RuntimeException (whether you should or shouldn't depends on the app).
That avoids the whole issue of creating an "if-else" block that tries to decode what happened during the try-catch. My second favorite would be Kenneth's solution - separate the try-catch tracking from the job status tracking.
How about this:
String finalStatus = "FAILED";
try {
setJobStatus(jobID, "NEW");
launchJob(job);
finalStatus = "SUCCESS";
} finally {
setJobStatus(jobID, finalStatus);
}
Maybe a bit cleaner than with the if. But I think I'll let the responsibility of knowing if the job execution was successfull or not to the Job itself.
My 2 cents.
Pablo.
I was going to suggest the same thing as mentioned in the first comment. Catching and rethowing RuntimeException is a common practice.
However, if you are concerned about and want to cover more than just runtime exceptions I'd suggest to go with Kenneth's suggestion. His example is nice and clean.
Just make sure that setting or getting the status does not cause any exceptions itself :-)
I think you take the problem in the wrong way.
Your job should be an object on itself, and it should manage it's own state, you shouldn't have calls for external methods in my opinion.
So basically :
Job myJob = new Job();
myJob.getJobProperties("myJob20070717.xml");
myJob.runJob(); // no throws here
if(myJob.getStatus() == Job.FAILED_STATUS)
log.error(myJob.getStatusMessage(),myJob.getStatusException());
What about this ?
I was also thinking along similar lines as Kilvaiden...
If you're not going to do anything with the exceptions coming out of "launchJob()", why not have that method do it's own error catching, and have it return a boolean indicating success/failure? (This assumes you have access to the "launchJob()" method...
I was about to suggest what kenneth wrote. I believe that internalizing the exception handling into a Job class changes the nature of the problem. What if we do want the exception and to set the status?
I don't think there's any way to avoid an Exception catch, but I thought this was a little cleaner:
boolean jobSucceeded = false;
try {
jobSucceeded = launchJob(jobId);
} catch (Exception e) {
jobSucceeded = false;
}
String jobStatus = jobSucceeded ? "SUCCESS" : "FAILURE";
setJobStatus(jobId, jobStatus);
Just my 2 cents...
Post a Comment