Using try/catch for preventing app from crashes
I would put this as a comment to some other answer, but I don't have the reputation for that yet.
You are correct in saying that it's bad practice, in fact what you posted shows different types of bad practice in regards to exceptions.
- Lack of error handling
- Generic Catch
- No intentional exceptions
- Blanket Try/catch
I'll try to explain all of those via this example.
try {
User user = loadUserFromWeb();
if(user.getCountry().equals("us")) {
enableExtraFields();
}
fillFields(user);
} catch (Exception e) {
}
This can fail in several ways that should be handled differently.
- The fields will not be filled, so the user is presented with an empty screen and then... what? Nothing - lack of error handling.
- There's no distinction between different types of errors, e.g. Internet problems or problems with the server itself (outage, broken request, corrupted transmission, ...) - Generic catch.
- You can not use exceptions for your own purposes because the current system interferes with that. - No intentional exceptions
- Unessential and unexpected errors (e.g. null.equals(...)) can cause essential code not to execute. - Blanket try/catch
Solutions
(1) First of all, failing silently is not a good thing. If there's a failure, the app won't work. Instead there should be an attempt to resolve the problem or a display a warning, for example "Could not load user data, maybe you're not connected to the Internet?". If the app is not doing what it's supposed to, that's way more frustrating for a user than if it just closes itself.
(4) If the User is incomplete, e.g. the country is not known and returns null. The equals method will create a NullPointerException. If that NPE is just thrown and caught like above, the fillFields(user) method will not be called, even though it could still be executed without problems. You could prevent this by including null checks, changing execution order, or adjusting the try/catch scope. (Or you could do save coding like this: "us".equals(user.getCountry()), but I had to provide an example). Of course any other exception will also prevent fillFields() from being executed, but if there's no user, you probably don't want it executed anyway.
(1, 2, 3)Loading from web often throws a variety of exceptions, from IOException to HttpMessageNotReadable exception to even just returning. Could be that the user isn't connected to the internet, could be that there was a change to a backend server or it is down, but you don't know because you do catch(Exception) - instead you should catch specific exceptions. You can even catch several of them like this
try{
User user = loadUserFromWeb(); //throws NoInternetException, ServerNotAvailableException or returns null if user does not exist
if(user == null) {
throw new UserDoesNotExistException(); //there might be better options to solve this, but it highlights how exceptions can be used.
}
fillFields(user);
if("us".equals(user.getCountry()) {
enableExtraFields();
}
} catch(NoInternetException e){
displayWarning("Your internet conneciton is down :(");
} catch(ServerNotAvailableException e){
displayWarning("Seems like our server is having trouble, try again later.");
} catch(UserDoesNotExistException e){
startCreateUserActivity();
}
I hope that explains it.
At the very least as a quick fix, what you could do is send an event to your backend with the exception. For example through firebase or crashlytics. That way you can at least see stuff like (hey, the main activity does not load for 80% of our users due to a problem like (4).
From the Android documentation:
Let's entitle it as -
Don't Catch Generic Exception
It can also be tempting to be lazy when catching exceptions and do something like this:
try { someComplicatedIOFunction(); // may throw IOException someComplicatedParsingFunction(); // may throw ParsingException someComplicatedSecurityFunction(); // may throw SecurityException // phew, made it all the way } catch (Exception e) { // I'll just catch all exceptions handleError(); // with one generic handler! }
In almost all cases it is inappropriate to catch generic
Exception
or Throwable (preferably not Throwable because it includes Error exceptions). It is very dangerous because it means that Exceptions you never expected (includingRuntimeExceptions
likeClassCastException
) get caught in application-level error handling.It obscures the failure handling properties of your code, meaning if someone adds a new type of
Exception
in the code you're calling, the compiler won't help you realize you need to handle the error differently.
Alternatives to catching generic Exception:
- Catch each exception separately as separate catch blocks after a single try. This can be awkward but is still preferable to catching all Exceptions.
Edit by author: This one is my choice. Beware repeating too much code in the catch blocks. If you are using Java 7 or above, use multi-catch to avoid repeating the same catch block.- Refactor your code to have more fine-grained error handling, with multiple try blocks. Split up the IO from the parsing, handle errors separately in each case.
- Re-throw the exception. Many times you don't need to catch the exception at this level anyway, just let the method throw it.
In most cases you shouldn't be handling different types of exception the same way.
Formatting / paragraphing slightly modified from the source for this answer.
P.S. Don't be afraid of Exceptions!! They are friends!!!
Of course, there are always exceptions to rules, but if you need a rule of thumb - then you are correct; empty catch blocks are "absolutely" bad practice.
Let's have a closer look, first starting with your specific example:
try {
View view = findViewById(R.id.toolbar);
}
catch(Exception e) { }
So, a reference to something is created; and when that fails ... it doesn't matter; because that reference isn't used in the first place! The above code is absolutely useless line noise. Or does the person who wrote that code initially assume that a second, similar call would magically no longer throw an exception?!
Maybe this was meant to look like:
try {
View view = findViewById(R.id.toolbar);
... and now do something with that view variable ...
}
catch(Exception e) { }
But again, what does this help?! Exceptions exist to communicate respectively propagate error situations within your code. Ignoring errors is rarely a good idea. Actually, an exception can be treated in ways like:
- You give feedback to the user; (like: "the value you entered is not a string, try again"); or to engage in more complex error handling
- Maybe the problem is somehow expected and can be mitigated (for example by giving a "default" answer when some "remote search" failed)
- ...
Long story short: the minimum thing that you do with an exception is to log/trace it; so that when you come in later debugging some problem you understand "OK, at this point in time that exception happened".
And as others have pointed out: you also avoid catching for Exception in general (well, depending on the layer: there might be good reasons to have some catch for Exception, and even some kinds of Errors at the highest level, to make sure that nothing gets lost; ever).
Finally, let's quote Ward Cunningham:
You know you are working with clean code when each routine you read turns out to be pretty much what you expected. You can call it beautiful code when the code also makes it look like the language was made for the problem.
Let that sink in and meditate about it. Clean code does not surprise you. The example you are showing to us surprises everybody looking at.
Update, regarding the update that the OP asks about
try {
do something
}
catch(Exception e) {
print stacktrace
}
Same answer: doing that "all over the place" is also bad practice. Because this code is also surprising the reader.
The above:
- Prints error information somewhere. It is not at all guaranteed that this "somewhere" resembles a reasonable destination. To the contrary. Example: within the application I am working with, such calls would magically appear in our trace buffers. Depending on context, our application might pump tons and tons of data into those buffers sometimes; making those buffer prune every few seconds. So "just printing errors" often translates to: "simply loosing all such error information".
- Then: you don't do try/catch because you can. You do it because you understand what your code is doing; and you know: I better have a try/catch here to do the right thing (see the first parts of my answer again).
So, using try/catch as "pattern" like you are showing; is as said: still not a good idea. And yes, it prevents crashes; but leads to all kind of "undefined" behavior. You know, when you just catch an exception instead of properly dealing with it; you open a can of worms; because you might run into myriads of follow-on errors that you later don't understand. Because you consumed the "root cause" event earlier on; printed it somewhere; and that somewhere is now gone.