Fix issues identified by Sonar

[goneall]

Sonar flagged a couple of issues in DownloadCache implementation.

Not sure why it flagged the closing of resources since it is covered in the finally clause, but I changed the implementation to use a try with resource approach.

[goneall]

@pmonks - Just FYI - I did a few changes to the original caching code to address issues found by the Sonar quality checks. Although none of the issues were significant IMO, I wanted to have a clean run on the quality checks.

I also added issue #210 to update the CI to check for these on pull requests which will hopefully flag these earlier.

[pmonks]

Cool! I forget - is try with resources supported on JDK 8? That's the only thing I can think of that might be problematic with that change.

Oh and you may have noticed that I'm a proponent of "single point of return only". I find that methods with more than one return statement tend to be harder to reason about, and as a consequence more likely to result in bugs as the code is maintained and enhanced over time. I realise that's not the dominant style in the rest of the library however.

One other comment: comparing to null directly (x == null) is slightly more performant than Objects.isNull(x), especially when you consider that that latter method is implemented as if (x == null) anyway.

None of these are significant or "must fix" concerns ofc.

[goneall]

is try with resources supported on JDK 8?

Yes - it is supported in JDK 8. I actually didn't find anything wrong with the prior implementation, but Sonar flagged it and suggested try ... with as a solution.

Oh and you may have noticed that I'm a proponent of "single point of return only". I find that methods with more than one return statement tend to be harder to reason about, and as a consequence more likely to result in bugs as the code is maintained and enhanced over time. I realise that's not the dominant style in the rest of the library however.

I usually do the early check and return early in the method if it helps reduce the nesting level. I lean more towards reducing nesting levels even at the expense of early exits from functions and loops - but you're right - it could lead to less maintainable code.

One other comment: comparing to null directly (x == null) is slightly more performant than Objects.isNull(x), especially when you consider that that latter method is implemented as if (x == null) anyway.

True - I do like the readability of the Objects.isNull. I'm OK with either approach in the library, I probably just added the function call more out of habit.

[pmonks]

As a smug lisp weenie nesting doesn't bother me at all - in fact without nesting nothing is happening. 😜