Retrieving multiple versions of source code for a file

Question

I have the following code that checks for three different properties. Unfortunately each call can throw up to four exceptions that I all have to catch. Can I some how make the code more readable by reducing the amount of try/catch?

public void retrieveSourceCode() {
 try {
 fileName = RestServices.getInstance().getClassName(getSourceCodeURI()); 
 } catch (ClientProtocolException e) {
 fileName = "not available";
 e.printStackTrace();
 } catch (IOException e) {
 fileName = "not available";
 e.printStackTrace();
 } catch (RestServicesException e) {
 fileName = "not available";
 e.printStackTrace();
 }
 if (!(fileName.endsWith(".jar") || fileName.endsWith(".svn-base"))) {
 try {
 sourceCode = RestServices.getInstance().sendGetRequestJsonTextToString(getSourceCodeURI());
 } catch (ClientProtocolException e) {
 sourceCode = "not available";
 e.printStackTrace();
 } catch (IOException e) {
 sourceCode = "not available";
 e.printStackTrace();
 } catch (RestServicesException e) {
 sourceCode = "not available";
 e.printStackTrace();
 }
 if (before == null) {
 beforeSourceCode = "no before source code available";
 } else {
 try {
 beforeSourceCode = RestServices.getInstance().sendGetRequestJsonTextToString(getBeforeVersionURI());
 } catch (ClientProtocolException e) {
 beforeSourceCode = "no before source code available";
 e.printStackTrace();
 } catch (IOException e) {
 beforeSourceCode = "no before source code available";
 e.printStackTrace();
 } catch (RestServicesException e) {
 beforeSourceCode = "no before source code available";
 e.printStackTrace();
 }
 }
 if (after == null) {
 afterSourceCode = "no after source code available";
 } else {
 try {
 afterSourceCode = RestServices.getInstance().sendGetRequestJsonTextToString(getAfterVersionURI());
 } catch (ClientProtocolException e) {
 afterSourceCode = "no after source code available";
 e.printStackTrace();
 } catch (IOException e) {
 afterSourceCode = "no after source code available";
 e.printStackTrace();
 } catch (RestServicesException e) {
 afterSourceCode = "no after source code available";
 e.printStackTrace();
 }
 }
 }
 getChangeSet().addAffectedFile(getFileName());
 }

• Andrew T Finnell: Someone should of pointed out that ClientProtocolException is an IOException making catching ClientProtocolException pointless in this code.
• Ryan Miller: You may not have control over it, but needing to check "before" and "after" for null is a little smelly to me.

Answer 1

If you upgrade to Java 7, you can use the new Multi-catch syntax. You could change the first block to:

 try {
 fileName = RestServices.getInstance().getClassName(getSourceCodeURI()); 
 } catch (ClientProtocolException | IOException | RestServicesException e) {
 fileName = "not available";
 e.printStackTrace();
 }

Essentially if there are many exceptions you catch and handle in the same way, you can use multi-catch

• Jesse C. Slicer: That's rather nifty. C# needs this. A lot.

Answer 2

These unhandled exceptions smell very bad. Storing error messages in the place of data also is not a good sign (like fileName = "not available").

Anyway, some notes:

1. You could extract out a

   RestServices restServices = RestServices.getInstance();
   

   variable.

2. You should create constants from repeating Strings, like "not available", "no before source code available" etc.

3. I'd extract out getFileName method for the first try-catch block, a getSourceCode for the second, and so on.

• Clockwork-Muse: Agreed - What's the point of catching an exception if you don't deal with it?

Answer 3

A try and catch block should be in their own method so as not to interrupt logic in another method.

private void getClassName(T URI, String error){
 T2 className;
 try{
 className = RestServices.getInstance().getClassName(URI);
 } catch (Exception e){
 className = error;
 e.printStackTrace();
 } finally{
 return className;
 }
}

and

private void sendGetRequestJsonTextToString(T URI, String error){
 T2 reply;
 try {
 reply = RestServices.getInstance().sendGetRequestJsonTextToString(URI));
 } catch(Exception e){
 reply = error;
 e.printStackTrace();
 } finally{
 return reply;
 }
}

This method now has no try-catch block interrupting the logic. Also, guard clause can be used in place of nested ifs.

public void retrieveSourceCode(){
 String error;
 error = "not available";
 filename = getClassName(getSourceCodeURI(), error);
 // this conditional can be extracted to a method, giving more readability
 // say, if(!isSourcecode(filename))
 if(fileName.endsWith(".jar") || !fileName.endsWith(".svn-base"))){
 return;
 }
 error = "not available";
 sourceCode = sendGetRequestJsonTextToString(getSourceCodeURI(), error);
 error = "no before source code available";
 if(before == null){
 beforeSourceCode = error;
 } else {
 beforeSourceCode = sendGetRequestJsonTextToString(getBeforeVersionURI(), error);
 }
 error = "no after source code available";
 if(after == null){
 afterSourceCode = error;
 } else {
 afterSourceCode = sendGetRequestJsonTextToString(getAfterVersionURI(), error);
 }
 getChangeSet().addAffectedFile(getFileName());
}

This still has some smells (as mentioned in another answer), but it does what the questions asking for: to simplify the try-catch block.

• Matt Ball: You've declared getClassName and sendGetRequestJsonTextToString as void, but then return values from them.

Answer 4

Considering that you do literally the exact same thing regardless of the type of exception, you can cut down the catch statements by catching type Exception instead of a specific subclass:

public void retrieveSourceCode() {
 try {
 fileName = RestServices.getInstance().getClassName(getSourceCodeURI()); 
 } catch (Exception e) {
 fileName = "not available";
 e.printStackTrace();
 }
 if (!(fileName.endsWith(".jar") || fileName.endsWith(".svn-base"))) {
 try {
 sourceCode = RestServices.getInstance().sendGetRequestJsonTextToString(getSourceCodeURI());
 } catch (Exception e) {
 sourceCode = "not available";
 e.printStackTrace();
 } 
 if (before == null) {
 beforeSourceCode = "no before source code available";
 } else {
 try {
 beforeSourceCode = RestServices.getInstance().sendGetRequestJsonTextToString(getBeforeVersionURI());
 } catch (Exception e) {
 beforeSourceCode = "no before source code available";
 e.printStackTrace();
 } 
 }
 if (after == null) {
 afterSourceCode = "no after source code available";
 } else {
 try {
 afterSourceCode = RestServices.getInstance().sendGetRequestJsonTextToString(getAfterVersionURI());
 } catch (Exception e) {
 afterSourceCode = "no after source code available";
 e.printStackTrace();
 } 
 }
 }
 getChangeSet().addAffectedFile(getFileName());
}

• brabec: But keep in mind that this will also catch unchecked RuntimeExceptions.

Answer 5

Java 7 syntax wins.

For Java < 7, catching the Exception base class works but it's unfortunate to stop unrelated exceptions from propagating. Here's a workaround to consider:

String getFileName() {
 try {
 return RestServices.getInstance().getClassName(getSourceCodeURI());
 } catch (ClientProtocolException e) {
 } catch (IOException e) {
 } catch (RestServicesException) {
 }
 return "not available";
}

• luiscubal: Note that you'll lose the stack trace.

Answer 6

You could use an if statement inside your catch. It could get ugly if you're trying to catch a lot of different exception types but it should be fine for a handful. Sorry for any errors in my syntax. It's been a while since I've used Java.

try {
 fileName = RestServices.getInstance().getClassName(getSourceCodeURI()); 
}catch (Exception e){
 if (e instanceof ClientProtocolException || e instanceof IOException || e instanceof RestServicesException) {
 fileName = "not available";
 e.printStackTrace();
 }else{
 throw e;
 }
}

• Marc-Andre: Welcome to Code Review! I'm glad you post an answer and hope you'll stay around. I'm not a big fan of this approach, this should get the job done, but is a lot of "boiler" plate IMHO.
• David DeMar: I mostly agree, hence why I'd only use it with a small number of exceptions. Otherwise multiple catch blocks is probably the way to go, although it looks like the latest Java version has solved this problem.