Criticism of my exception handling strategy
I've been doing object oriented programming for most of the last 7 years using Java at this time. Some things I'm sure I have a great understanding of, like the most useful design patterns. In fact, the following code allowed me to run a small system in the daytime that would handle one particular instance, which we are ready to implement now, being flexible enough to handle the future requirements that I was told about:
public void importAndArchive(File detectedFile) throws FileNotFoundException, IOException {
File workingCopy = returnWorkingCopy(detectedFile);
List<String[]> csvData = csvData(workingCopy);
setHeaderFields(csvData.get(0));
importData(csvData); //subclass will implement this abstract method
archiveWorkingCopy(workingCopy);
}
I am not showing above to brag about my understanding of the template method, but rather a starting point for discussing the gap that I have at my best, which is blatant in light of my OO design abilities. And this gap is a systematic approach to exception handling. Indeed, you can see in this method signature that I am currently rethrowing some exceptions, but in fact, I continued to root out the same in another corner of the application.
Before I go much further, since this is my first attempt at being particularly systematic, I would like some confirmation of what I have done so far. Our application goes through a bunch of files, "processes" them, and "archives" them. Pretty standard rate. One of the decisions I made to get the prototype up and running as quickly as possible is as follows. The application gets initialized based on the data in the properties file (ResourceBundled). Various methods in the ResourceBundle API throw unchecked exceptions, but I don't handle them at all yet, on the assumption that they will prevent the application from starting, and may be enough on the stack for a while.
However, I chose a CSV handling library that throws checked exceptions. After that, NetBeans made it easy to propagate these exceptions to begin with :) Below is a method that it has since reworked that actually handles these exceptions:
private String detectImportHandler(File detectedFile) throws Exception {
String[] firstLine = null;
try {
/*
* CSVReader throws checked exceptions
*/
CSVReader csvReader = new CSVReader(new FileReader(detectedFile));
firstLine = csvReader.readNext();
csvReader.close();
}
catch(Exception x) {
throw new Exception("CSVReader unable to process file: " + x.getMessage());
}
try {
return firstLine[1];
}
catch(Exception x) {
/*
* since we're re-throwing CSVReader checked exceptions it seems easiest
* to also re-throw null-pointer and/or array-out-of-bounds errors
*/
throw new Exception(
"First line null or did not have importHandlerType field: " + x.getMessage());
}
}
The above method is called this way inside a loop that processes files:
try {
importHandlerType = detectImportHandler(detectedFile);
}
catch(Exception x) {
unusableFileErrors.put(detectedFile.getName(), x.getMessage());
continue;
}
FileErrors is a map, and I figure that when I'm done iterating over the files, I should be able to use that map and its associated files designed for higher level work, such as logging, moving files somewhere else in system, etc.
Anyway, I continued long enough. I have ordered the book Reliable Java , and I hope that between it and the SO community, I can improve on this forgotten edge of my ability. I've seen other similar questions on SO, but I suppose I should also ask for specific guidance in the context of real code.
source to share
A few notes:
-
When wrapping an e exception, you just use e.getMessage (). Instead, I suggest you use a new exception (message, e). This will set the reason for the exception, so the entire stacktrace will also contain the stacktrace of the original exception. This can be very useful for debugging the reason for your exception.
-
Instead of throwing an exception, I suggest you add more explicit exceptions, eg. FileNotFoundException. Use exceptions defined by Java APIs when they make sense.
-
Instead of catching the Exception, I suggest you explicitly specify which exceptions to catch.
-
(Optional replacement for 2) Some developers prefer - instead of defining an exception class for each kind of exception - throw a generic exception containing an error key indicating the type of exception, eg. new detailed exception (GeneralBusinessErrors.PARSING_ERROR). This makes it easier to assign language-specific resource files for business errors.
-
It may be helpful to add debug information to your exception. For example, if the file is not found, it might be information such as the file you were trying to open. This is very useful in maintenance where you cannot reproduce the problem and / or debug with a debugger.
-
It can be helpful to have your system log any unchecked exceptions thrown that were not caught. Generally speaking, it can be helpful to log any exception that is thrown (caught or not).
source to share
I usually use the catch-rethrow approach if I wanted to translate implementation exceptions into exceptions that I wanted to expose through my API .
For example, suppose the import method from your example accepts URL
, not File
. If this url was related to File
, I would not want to expose FileNotFoundException
to the API (this is an implementation detail), but rather catch and reinstall that (like how ImportException
), but still bind to the underlying FileNotFoundException
reason (as suggested by David).
Also, I usually don't catch and discard thrown exceptions (e.g. NullPointerException
), although in some cases (e.g. Spring DataAccessException
).
source to share
There are some very good answers already, but I wanted to add an additional thought.
There is not only one error handling strategy that works for all cases.
In general, you must consciously choose between two fundamentally opposite cases. The choice will differ depending on what you are building and what layer in the application you are currently working on. These two alternatives:
-
Crash fast. The error immediately occurs, you collect all the information you have and ensure that it reaches someone who can do something about it - either in your code, or by throwing an exception, or perhaps by calling support, who can get code set before data corruption occurs. Masking the error, or not being able to get enough information about it, will make it difficult to determine what went wrong.
-
Be bulletproof. Carry on because stopping will be a disaster. Of course, you still need to collect information about the error and report it somehow, but in this case, you need to figure out how you are going to recover and continue.
Many errors are introduced because the programmer adopts the second strategy when they should have adopted the first. If it's not clear how you should recover from the situation, you should probably throw an exception and let the caller make an intelligent choice - they may have a better idea of what to do than you, but they should have enough information. to decide what it is.
Another helpful tip is to ensure that the exception you are throwing makes sense at the API level. If your API is for html data rendering, you don't want to see a database exception, simply because someone decided to enable audit trail. Such cases are most often handled using exception chaining - throw new ApiException(causeException);
eg.
Finally, I want to give me the option to uncheck the boxes and check for exceptions. I like to reserve unchecked exceptions for situations that mean that the programmer has made a mistake. For example, passing null to a method that requires an object reference. If an error can occur even if the programmer is perfect (FileNotFound if the file has been deleted since the programmer has verified its existence, for example), a checked exception is usually considered. Basically, the programmer who creates the API is saying that "even if you entered all the inputs correctly, then this problem can occur and you may have to deal with it."
source to share
I already gave @Kolibri a vote, so read it first, but I need to add a few things.
Exception handling is a bit of a black art, and the expected practices vary from language to language. In Java, consider method signature as a contract between your method and the caller.
If the caller violates his end of the contract (for example, passes in "detectedFile" as null), you want to throw a failed validation exception. Your API gave them a contract and they clearly violated it. Their problem. (Ignore the FileNotFoundException checked for now, this kind of beef I have with Java)
If the caller passes the correct data and you cannot fulfill their request, however, when you threw an exception thrown. For example, if you cannot read their file due to a disk error (like an IOException), then you are breaking your end of the contract and it makes sense to throw a checked exception. Checked exceptions warn the caller of all the possible things that could go wrong when calling your method.
To that end, it depends on what granularity you want to give the user to handle your exceptions. Throwing "Exceptions" is usually not desirable because, as others have said, it doesn't give you any granularity in handling what happened. The caller will not immediately know if they violated the contract or your method, because Exception includes both checked and unchecked exceptions. In addition, it is possible that your caller wants to handle FileNotFoundException and IOException differently in their code. If you just throw an Exception, they lose the ability to do it easily. Defining your own exceptions can expand on this concept and allow callers of your methods to handle exceptions from your method in a more granular manner. The key point here isthat if you throw 10 different exceptions and the caller doesn't care about the difference, they can just "catch (Exception e)" and catch them all in one shot. If you just "throw the exception," the user no longer has a choice as to whether they handle different exceptions differently or just catch them all.
I also very much agree with the comments about using "throw new XXXException (message, e)" instead of "throw new Exception (message + e.getMessage ())". If you use the first method, you will keep the internal stack trace back to the original error. If you use the second method, the stack trace will now only return to your "throw new Exception" line, and you will have to do more debugging to figure out what caused this exception. It has bitten me in the past.
Finally, don't be afraid to use Exception.printStackTrace () in the catch if it helps you. Users hate to see stack traces and you should handle exceptions where you can, but if it's really an unexpected exception and having a stack trace helps you debug significantly in the future, just print the stack trace.
Wow, it looked like an essay. I will stop now. Good luck!: - D
======================
Addendum: I just noticed @pjp's comment and it repeats ... You should definitely call this method ".close ()" in the "finally" block after your "catch" statement. Otherwise, in the event of an exception, your method will return without immediately closing the resource that you allocated and opened.
source to share
I think there is another dimension of the problem that is not yet covered. And that is that exception handling is not only a coding technique, but an error condition and what you want to do. Java checked exceptions are a mixed bag because it forces you to think about error conditions when you might not want to, causing some developers to do something worse than doing nothing in their quest to get the compiler out of their way. (Exceptions have the opposite problem - they make developers forget about error handling even in very common scenarios, such as a lost network connection).
There are three classes of exceptions, which correspond to the three types in Java: checked exceptions, exceptions, and runtime errors.
Errors should only be handled at a very high level in the application and should generally be considered unrecoverable. While OutOFMemoryError is somewhat recoverable in practice, there are too many errors in the sequence of code that you don't think about (such as initializing an object) that can leave things in a bad state, which you should only try if you can split the code (for example , on the application server).
Runtime exceptions in short hand should be deterministic and will always fail on the same inputs. For example, if you pass a null reference to a method that does not resolve null (NullPointerException exception), or if you execute Integer.parseInt on a given string, the result is the same every time.
Checked exceptions, again in the short hand definition, are things that you cannot know in advance if they go wrong or not. FileNotFoundException, for example the file might be there when you created the File object, you might even check, but milliseconds later, something might remove it from under you.
If you design and react to exceptions with these categories (realizing that not all API designers follow them), for example, XML exceptions are checked even though the behavior is deterministic for a given XML input and of course Spring just decided to avoid thrown exceptions altogether), then you have clear ways to think about what kind of error handling you need. What you need to do when a network connection occurs is very different (depends on your project, of course) from what you do if the JVM is running out of memory and both are very different from what happens if you need to parse Integer, but doesn't get it from user input.
Once you think in these terms, what you do in relation to exception handling will more naturally follow from what type of error handling and reliability you are trying to achieve.
source to share
Small side note: I would suggest linking the actual reason rather than just adding the original post; otherwise, you will lose the stack trace and debugging will be more painful than necessary.
Other than that: I don't see much value in try / catch blocks inside detectImportHandler()
. Since the method is already throwing Exception
, there is no need to re-wrap whatever was thrown , CSVReader
but NullPointerException
will hit the caller with try / catch just as well as anything else.
What would be more valuable would perhaps be a suggestion finally
that closes FileReader
(and / or CSVReader
) if an exception can be thrown readNext()
either by the CSVReader
constructor.
source to share
Catching and throwing a plain old is Exception
bad - you get all runtime exceptions whether you want them or not (maybe not).
I am trying to throw checked exceptions that make sense in the context of the method they are coming from. For example, if I had loadCustomer(long id)
, I would choose ObjectLoadException
rather than SQLException
.
And yes, you read that correctly, my preference is checked by exceptions - I like the fact that the consumer of a method has to explicitly decide what to do when an exception is thrown, and improves readability through an imaginary call stack when reading imho code.
source to share
I usually handle exceptions in one of two places:
- If I can handle the exception without needing to know the user, I handle it as close to the error as possible.
- If the error requires some kind of output (logging, popup dialog, writing a message to stdout), I allow it to go to some centralized point, after which it will be caught and distributed to the appropriate output engines.
This seems to work well enough for me.
source to share