HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Getting the latest hash from a file

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thelatestfilegettinghashfrom

Problem

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;
}

Solution

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.

Code Snippets

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

Context

StackExchange Code Review Q#82510, answer score: 3

Revisions (0)

No revisions yet.