I have been working on an Android app which uses try/catch frequently to prevent it from crashing even on places where there is no need. For example,

A view in xml layout with id = toolbar is referenced like:

try {
    View view = findViewById(R.id.toolbar);
}
catch(Exception e) {
}

This approach is used throughout the app. The stack trace is not printed and it's really hard to find what went wrong. The app closes suddenly without printing any stack trace.

I asked my senior to explain it to me and he said,

This is for preventing crashing in production.

I totally disagree with it. To me this is not the way to prevent apps from crashes. It shows that developer doesn't know what he/she is doing and is in doubt.

Is this the approach being used in industry to prevent enterprise apps from crashes?

If try/catch is really, really our need then is it possible to attach an exception handler with UI thread or other threads and catch everything there? That will be a better approach if possible.

Yes, empty try/catch is bad and even if we print stack trace or log exception to server, wrapping blocks of code in try/catch randomly across all the app doesn't make sense to me e.g. when every function is enclosed in a try/catch.

share
6  
Not answering your question, but never sliently swallow exceptions catch(Exception e){} - this comment made sense before the edit to the question – Scary Wombat yesterday
1  
not answering the main question, but shouldn't it be findViewById(R.id.toolbar); instead? – Jonathan Darryl yesterday
1  
2  
Even stacktrace is not printed. see my original comment – Scary Wombat yesterday
14  
Ah, the infamous ON ERROR RESUME NEXT – Deduplicator 21 hours ago

12 Answers 12

up vote 25 down vote accepted

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.

share
    
What if I print stack trace? I still don't think I need try/catch here. – mallaudin yesterday
    
I will read it later. Can please check the updated question? – mallaudin 22 hours ago
3  
I have to admit I dont get your edit. Not a real idea what you are doing there; or where you are getting with this. – GhostCat 22 hours ago
1  
@GhostCat The OP says on a comment in another answer that the exception blocks already contain logging calls. The question misrepresents what's really going on, so I don't think accusing the senior dev of being incompetent is helpful here. I think there's more going on than just what the OP is giving us. – jpmc26 18 hours ago
1  
@jpmc26 You are probably right; I was in yeeha-100-rep-before-lunch-rush and overdid. Removed that part; and beyond that; maybe the issue is less about the seniority of the senior but the juniority of the junior ;-) – GhostCat 18 hours ago

Pasting a paragraph from Android documentation -

I'll 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 (including RuntimeExceptions like ClassCastException) 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 a generic Exception:

  • This one is my choice - Catch each exception separately as separate catch blocks after a single try. This can be awkward, but it is still preferable to catching all Exceptions. Beware repeating too much code in the catch blocks.
  • Refactor your code to have more fine-grained error handling, with multiple try blocks. Split up the I/O from the parsing, handle errors separately in each case.
  • Rethrow 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.

share
1  
if you catch each exception individually, make sure to avoid continuing the execution in an uncertain state (avoid things like Object a; try { a = thing(); } catch (Exception e) {...} try { a.action(); } catch (Exception e) {...})) – njzk2 11 hours ago
    
To be fair, catching generic Exception could be useful as part of a "log-before-crashing" system, although it should rethrow in that case. – Justin Time 6 hours ago

It's definitely a bad programming practice.

From the current scenario, if there are hundreds of try catch like this, then you won't even know where the exception occurs without debugging the application, which is a nightmare if your application is in production environment.

But you can include a logger so that you get to know when an exception is throws (and why). It won't change your normal workflow.

...
try {
    View view = findViewById(R.id.toolbar);
}catch(Exception e){
    logger.log(Level.SEVERE, "an exception was thrown", e);
}
...
share

I have been developing android apps for the past 4-5 years and never used a try catch for view initialisation.

If its a toolbar do like this

Toolbar toolbar = (Toolbar) findViewById(R.id.toolbar);

eg:- To get a TextView from a view(fragment/dialog/any custom view)

TextView textview = (TextView) view.findViewById(R.id.viewId);

TextView textview = (TextView) view.findViewById(R.id.viewId);

instead of this

View view = findViewById(R.id.toolbar);

A view object has minimum reach compared to its real view type.

Note:- May be it crashed because the view was loaded. But adding try catch is a bad practice.

share
    
In many case casting the view is not necessary. There are a few methods that you have on View that can be already useful (like setVisibility()), without you specifying exactly which class is used. (for example, in the case of a layout, which is susceptible to be changed from time to time) – njzk2 11 hours ago

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.

  1. Lack of error handling
  2. Generic Catch
  3. No intentional exceptions
  4. 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.

  1. The fields will not be filled, so the user is presented with an empty screen and then... what? Nothing - lack of error handling.
  2. 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.
  3. You can not use exceptions for your own purposes because the current system interferes with that. - No intentional exceptions
  4. 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).

share

This is bad practice. Other answers have said that but I'd think it's important to step back and understand why we have exceptions in the first place.

Every function has a post-condition – a set of things that must all be true after that function executes. For example, a function that reads from a file has the post condition that the data in the file will be read from disk and returned. An exception, then, is thrown when a function has not been able to satisfy one of its post-conditions.

By ignoring an exception from a function (or even effectively ignoring it by simply logging the exception), you're saying that you're ok with that function not actually doing all the work it agreed to do. This seems unlikely – if a function does not run correctly, there is no guarantee that what follows will run at all. And if the rest of your code runs fine whether or not a particular function runs to completion, then one wonders why you have that function in the first place.

[Now there are certain cases where empty catches are ok. For example, logging is something that you might justify wrapping in an empty catch. Your application will probably run fine even if some of the logging can't be written. But those are special cases that you have to work really hard to find in a normal app.]

So the point is, this is bad practice because it doesn't actually keep your app running (the supposed justification for this style). Maybe technically the OS hasn't killed it. But it's unlikely that the app is still running properly after simply ignoring an exception. And in the worst case, it could actually be doing harm (e.g. corrupting user files, etc.).

share

Yes, try/catch is used to prevent app from crashing but You certainly don't need try/catch for fetching a view from XML as depicted in your question.

try/catch is generally used while making any http request, while parsing any String to URL, creating URL connections, etc. and also make sure to print stack trace. Not printing it doesn't make much sense in surrounding it with try/catch.

share

It's bad practice to use catch(Exception e){} because you're essentially ignoring the error. What you probably want to do is something more like:

try {
    //run code that could crash here
} catch (Exception e) {
    System.out.println(e.getMessage());
}
share

We pretty use much your same logic. Use try-catch to prevent production apps from crashing.

Exceptions should be NEVER ignored. It is a bad coding practice. The guys maintaining the code will have a really hard time localizing the part of code that raised the exception if they are not logged.

We use Crashlytics to log the exceptions. The code will not crash (but some functionality will be disrupted). But you get the exception log in the dashboard of Fabric/Crashlytics. You can look at these logs and fix the exceptions.

try {
    codeThatCouldRaiseError();
} catch (Exception e) {
    e.printStackTrace();
    Crashlytics.logException(e);
}
share
    
Yes. That's what people are doing here but I disagree with it. That's why I posted the question. Can you come up with something more logical? – mallaudin yesterday
4  
@mallaudin Don't post example code that misrepresents the code you're asking about. It can drastically change the answer you get. – jpmc26 18 hours ago
    
@jpmc26 yes. I have seen that in answers. – mallaudin 18 hours ago

While I agree with the other responses, there is one circumstance I have repeatedly encountered where this motif is marginally tolerable. Suppose someone wrote a bit of code for a class as follows:

private int foo=0;

    . . .

public int getFoo() throws SomeException { return foo; }

In this circumstance, the 'getFoo()' method cannot fail - there will always be a legitimate value of the private field 'foo' to be returned. Yet someone - probably for pedantic reasons - decided that this method should be declared as potentially throwing an Exception. If you then try to call this method in a context - e.g an event handler - which does not allow an exception to be thrown, you are basically forced to use this construct (even then, I agree that one should at least log the exception just in case). Whenever I have to do this, I always at least add a big fat comment 'THIS CANNOT OCCUR' next to the 'catch' clause.

share

A nice article on the topic:

Why your app should crash

Too many times I've seen developers trying to avoid crashes at all cost. Actually, sometimes you want your app to crash. Jeroen Mols explains why and gives some practical tips.

(from Android Weekly)

share
    
Good article. I wish all Project Managers shared the same view :) – K Neeraj Lal 16 hours ago
1  
This alone does not answer the question. While the linked content may answer the question, you must include the essential parts of the answer here and provide the link for reference. Link-only answers become invalid when the linked page inevitably changes. – cpburnz 12 hours ago
    
While this link may answer the question, it is better to include the essential parts of the answer here and provide the link for reference. Link-only answers can become invalid if the linked page changes. - From Review – xlm 1 hour ago

This is bad for multiple reasons:

  1. What are you doing that findViewById throws an Exception? Fix that (and tell me, because I've never seen this) instead of catching.
  2. Don't catch Exception when you could catch a specific type of exception.
  3. There is this belief that good apps don't crash. That's not true. A good app crashes if it must.

If an app goes into a bad state, it is much better for it to crash than for it to chug along in its unusable state. When one sees an NPE, one shouldn't just stick in a null check and walk away. The better approach is to find out why something is null and either stop it from being null, or (if null ends up being a valid and expected state) check for null. But you have to understand why the issue occurs in the first place.

share

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.