Fixed a number of warnings

[oscargus]

Got rid of a number of warnings:

• <>
• static methods
• commented empty blocks
• general clean up

[lenhard]

Why do you turn this static? It is not called from a static context.

[lenhard]

I find this PR very valuable (we need such improvements). The only thing I am unsure about is turning various methods static (see above). Can you explain the rationale behind this? Why is static needed here?

[koppor]

The static conversion is because Eclipse warns if methods could be made static. On the one hand, we could add @suppresswarning if we are sure that Eclipse is wrong. On the other hand, we could just turn off the static warning. I don't have strong a opinion about that. I currently lean towards making methods static if possible to show that they don't access instance variables.

[oscargus]

As @koppor correctly pointed out, this is based on Eclipse warnings and Eclipse warnings only. I've trusted it so far, but recently I've noted that some warnings disappear when I fix other, to me seemingly unrelated, warnings, so maybe I should read up a bit...

[lenhard]

I tend to disagree with the warning and would prefer non-static versions. The problem with the use of static is that it leads to a higher degree of coupling within the system. Every user of a static method uses the very same instance (that is, the class) and this transitively couples two users although there might be no need for it.

Don't get me wrong, the code works with both versions. Especially if the class is instantiated only once, as is the case in the above line I commented at, there is no difference execution-wise. It is merely a "design smell". I have the opinion that a design with a lot of use of static is "worse" than a more object-oriented design. If we consistently favor static over non-static, we end up with a purely imperative program, which means we are propably using the wrong programming language.

@JabRef/developers This is about a coding guideline and concerns everyone. So what are the opinions of the remaining people? Should we consistently fix the Eclipse warning regarding the use of static, or should we turn it off?

[oscargus]

OK, I've read up and see the point. I'd say that maybe one should go through all the static methods and remove the static keyword and add warning removal in case it does not make sense to access the method in a static way.

For reference: I've done this in the earlier cleanups as well, so it may be better to go over it once and for all (although I can of course start by changing these ones back).

[matthiasgeiger]

I agree with @lenhard - we should try to reduce the usage of static methods and not introduce new ones.
Quick rule of thumb:

Static is sensible in Utility classes which provide independent small methods, e.g, getIntValue(...). Static should be avoided in more complex "logic classes" - even if only static or no members are accessed.

Objections? @lenhard

I don't like @suppressWarnings annotations - so would either turn off this warning completely - or just ignore those warnings "manually", i.e., think about it and decide whether the usage is okay or not.

[lenhard]

@matthiasgeiger full ack 👍

[koppor]

Could you review eb44184 and efce4ec (unrelated, but I think, it's important, too)

[lenhard]

@koppor Looks got to me. I imported the project into Eclipse and am not getting the warning at the above position. I also agree with you on the usage of "this": it should not be mandatory.

However, I found another Eclipse warning, we should be aware of here. In the following line

new FocusRequester(((BasePanel) JabRef.jrf.tabbedPane.getComponentAt(0)).mainTable);

Eclipse warns that The allocated object is never used. The warning is a false positive. FocusRequester starts itself in a separate Thread in its constructor (which is unsafe publication and a potential bug, but that is a different issue) and it does not matter that it is not used at this position. I don't think we should ignore the unused warning, but we should not blindly delete every variable for which it occurs. Instead, we should add a @SuppressWarnings("unused"), if it is a false positive.

[oscargus]

I have removed the statics introduced in this PR.

[oscargus]

Rebased on current master so it should be possible to merge this (unless any other issues arise).

[matthiasgeiger]

@param should be removed here completely

[matthiasgeiger]

Huh.... requires quite some time to scroll through all your changes 😉

As you can see above I had a few remarks.

Apart from those: Good job! Should be ready to merge (which should be easy after your rebasing - bonus kudos for this 🏆)

[oscargus]

Thanks for the review! Should be good to go now.