Getting the latest hash from a file

Question

I'm trying to write clean code, but I'm not so good with exception handling. How can I improve this method? Or is it good enough already?

This method gets a hash from a file.

public String getLatestHash() {
 String hash = "none";
 URL packURL = null;
 try {
 packURL = new URL(ConfigManager.get("play_url") + "/" + HTMLParser.getPackName());
 } catch (MalformedURLException e) {
 logger.severe("invalid URL!");
 logger.severe(e.getMessage());
 }
 try (InputStream inputStream = packURL.openConnection().getInputStream()) {
 hash = DigestUtils.md5Hex(inputStream);
 } catch (IOException e) {
 e.printStackTrace();
 }
 return hash;
}

• Sotirios Delimanolis: What happens to packURL if you get a MalformedURLException?
• user2997204: @SotiriosDelimanolis It will stay null?
• Simon Forsberg: @SMA Code Review is, just like Stack Overflow, Programmers, and all other Stack Exchange sites, not a forum!
• Vogel612: and things blow up, eh? I strongly suggest you revisit the creation of your inputStream to prevent NullPointerExceptions
• fge: Make this method throw an IOException instead of having an NPE thrown when the URL is null

Answer 1

Why do you use logger.severe(...) in your MalformedURLException, but in your IOException you use e.printStackTrace()? Pick one and stick with it; this can include, for example, totally removing the printStackTrace() and replacing it with a comment that explains that you don't need to log it because [reason]. However, I'd suggest logging everything. It's a lot easier to find bugs if you log everything.

try (InputStream inputStream = packURL.openConnection().getInputStream()) {

And when you get a MalformedURLException in the try/catch before, your user is going to receive a very confusing NullPointerException instead of a more palatable (possibly custom) exception that means something in context. You can always catch (NullPointerException e) { throw /*your exception here*/; } to 'change' the exception being thrown (though be sure to use better indenting)

You define packURL right before you use it, but you define hash way at the beginning. Be consistent -- move String hash = "none"; to the line just before the relevant try.

Why is the default hash "none"? Wouldn't an empty string make more sense?

Aside from that, though, it looks about as good as it'll get. The multiple separate try/catch blocks are messy, but as far as I can tell that's the best way to do it.

Answer 2

The first solution one should think about is

public String getLatestHash() throws MalformedURLException, IOException {
 URL packURL = new URL(ConfigManager.get("play_url") + "/" + HTMLParser.getPackName()); 
 try (URLConnection connection = packURL.openConnection()) {
 try (InputStream inputStream = connection.getInputStream()) {
 return DigestUtils.md5Hex(inputStream);
 }
 }
}

Trying to do something which you can't do, is determined to fail. And you probably can't handle any exception here as logging and returning a nonsense is no handling. Maybe you can, but you didn't tell us why some dummy result is acceptable.

In case the above exceptions are inappropriate, you may want to wrap them in another exception, maybe HashFailedException.

Note that you need two tries with resources in order to ensure that both the connection and the inputStream get closed.