Minor refactorings to Bukkit sub-project

[mangstadt]

Hello! I was looking for a Java project to contribute to and stumbled upon this one. :) I tried to separate out each change into its own commit. They are mostly suggestions related to readability, with one potential bug fix. Hope it helps! :)

[stephan-gh]

Thanks for your effort and keeping the commits cleanly separated! This makes it easier to take some of them already. Couple of comments for each commit separately:

• Use ternary operators: I think using ternary operators does not necessarily make the code easier readable/understandable. The original code is kind of easier to read because first all the failure cases are handled and then the result is returned.
• Refactor to make code self-documenting: This might be personal preference but I kind of prefer the original code with the comments over the boolean variables. Just seems like an odd code style to me.
• Fix inconsistent formatting, Unnecessary null check: I applied these two (after adding a Bukkit: prefix to the commit message), thanks!
• Create unique variable names for each getter call: Yeah, I suppose using the same variable over and over again looks a bit confusing. I would be open to apply this, but similar code is used in all subprojects, so only if we keep this consistent. Maybe you can make a commit that changes this in all modules?
• Consolidate repeated method calls: The code actually ends up being longer (and imo a bit worse readable), so I'm not convinced this is worth it. :)
• Fix spelling mistake, Refactor to use "Optional.orNull()": Applied, thanks!
• Swap variables in string comparison: But player.getWorld() is defined as @NotNull, so this should be impossible?
• Mark method body as being empty on purpose: I actually had some unfinished local changes that add something to this method, pushed that now. :)

[mangstadt]

Thanks for your thoughtful responses! Glad that some of the changes were useful. :)

Refactor to make code self-documenting: This might be personal preference but I kind of prefer the original code with the comments over the boolean variables. Just seems like an odd code style to me.

It find this style useful in certain, non-intuitive situations. For example, in the BukkitPlugin.reloadFaviconCache method, to me it's non-intuitive that passing in "null" deletes the cache. By assigning the null check to a well-named variable, it makes it more apparent to me as to why the null check is being done. (On second thought, I wonder if it might make more sense to have a separate deleteFaviconCache method so you don't have to do the null check at all?)

I also find it useful when an if statement is complex, like in the BukkinPlugin.reloadCaches method. It makes the if statement easier to read IMO because you are moving all of the complex boolean logic to its own variable. :)

Swap variables in string comparison: But player.getWorld() is defined as @NotNull, so this should be impossible?

I didn't realize it had that annotation. Never mind!

Create unique variable names for each getter call: Yeah, I suppose using the same variable over and over again looks a bit confusing. I would be open to apply this, but similar code is used in all subprojects, so only if we keep this consistent. Maybe you can make a commit that changes this in all modules?`

I will work on this.

[stephan-gh]

It find this style useful in certain, non-intuitive situations. For example, in the BukkitPlugin.reloadFaviconCache method, to me it's non-intuitive that passing in "null" deletes the cache. By assigning the null check to a well-named variable, it makes it more apparent to me as to why the null check is being done. (On second thought, I wonder if it might make more sense to have a separate deleteFaviconCache method so you don't have to do the null check at all?)

Hmm yeah, I suppose the naming is a bit "unfortunate" there. The cache code is really old and to be absolutely honest I don't fully remember how it works exactly either, so I would kind of prefer to leave it alone so things don't explode. (As explained in a couple of issues in this repository, I never managed to finish up the ServerListPlus 4.0 rewrite that would have had somewhat nicer code.)

[mangstadt]

The cache code is really old and to be absolutely honest I don't fully remember how it works exactly either, so I would kind of prefer to leave it alone so things don't explode

Whoops, just re-read this sentence. Please disregard https://github.com/Minecrell/ServerListPlus/pull/354 if you'd rather not touch that code!