Replaced some getField and fixed some bugs

[oscargus]

Thanks to @Siedlerchr reminding me about the brilliance of Optional, I decided to try and convert some BibEntry.getField to BibEntry.getFieldOptional in a "proper" way. I managed in a few positions, but more importantly i reinstalled Find bugs as a consequence of this.

I found a few quite bad bugs, such as NPEs when clicking "Browse" in the journal management dialog with an empty text field. Also, there where some other example of introducing requireNonNull without really checking that all existing calling functions did call without a null hard coded...

@Siedlerchr as I know that you are a big fan of Path, Find bugs complains that some method there is quite likely to return null (I think getPath()), so stacking a number of method calls is claimed to be risky.

Still a few to be fixed (appending a database with explicit groups will most likely not work at the moment), but I leave that to the persons introducing them.

• Manually tested changed features in running JabRef

[Siedlerchr]

What the hell is that for a method?

[oscargus]

The marvel of JabRef. 😄

[Siedlerchr]

Regarding the fileDialogs and eventual nulls, please don't change that much, as I am rewriting them completely in #1336. I will only make merging harder ;)

[oscargus]

I only changed where the extension List was set to null and where the un-null setting of a File was optional leading to an NPE. The "possible" nulls I leave. :-)

[oscargus]

Added CHANGELOG entries for the NPE bugs I corrected. @JabRef/developers : I think we should try and merge those fixes before next release. ;-)

[Siedlerchr]

The equals methods looks a bit weird, it should follow a more clear pattern. A possible good solution would be this:

        @Override
        public boolean equals(Object other)
        {
            if (other == null) // Null-check
                return false;

            if (this == other) // reflexivity 
                return true;

            if (this.getClass() != other.getClass()) // Type safety 
                return false;

            final CustomImpoter importer = (CustomImporter) other;
            return Objects.equals() && ...
        }

[lenhard]

That is correct. Implementations of equals should always follow a very narrow contract. @oscargus: If you are interested, this is excellently described in Effective Java 2nd Edition, Item 8: "Obey the general contract when overriding equals" (I won't link a pdf version here, but I am sure you can find one if you desire).

Most IDEs can auto-generate you a good implementation of equals these days.

[oscargus]

I checked the 35 equals methods of JabRef and there is clearly not a single pattern in how they are implemented. I'm not sure if getClass is the universal solution here. Sure, it is faster than instanceof but the use is dependent on the class, see e.g. http://stackoverflow.com/questions/4989818/instanceof-vs-getclass

Might be good to get some consensus on this and add it to the code guidelines.

The other interesting question is if this should stop merging. Now I have updated it, but in this PR this was a very minor part and the alternative would have been to just not introduce the null check in the first place. The question is really if that would have been a better alternative? It is another thing if issues are introduced by incomplete code. To me it is not a problem, but I am thinking how new contributors react. There is a risk that they won't fix these minor issues if they are part of a bigger one or that they will stop contributing.

[lenhard]

Regarding equals consensus is easy. Effective Java sets the standard here. Let me quote the rules for implementing equals (for a deeper explanation, please refer to the book):

1. Use the == operator to check if the argument is a reference to this object. If so, return true.
2. Use the instanceof operator to check if the argument has the correct type. f not, return false.
3. Cast the argument to the correct type.
4. For each “significant” field in the class, check if that field of the argument matches the corresponding field of this object. If all these tests succeed, return true otherwise, return false.
5. When you are finished writing your equals method, ask yourself three questions: Is it symmetric? Is it transitive? Is it consistent?

Also, note:

• Always override hashCode when you override equals (hashCode also has very strict rules)
• Don’t try to be too clever
• Don’t substitute another type for Object in the equals declaration

Regarding JabRef, most equals methods are probably broken in one way or the other. We should gradually replace these with correct implementations. From my point of view, incorrect equals methods should prohibit the merging of a PR. It is not so hard to implement a correct one (since most IDEs generate it), so it something that we can ask from people in my point of view. If you do it, it should be correct. As a side note, your implementation does not fully follow the rules, the first null check is not necessary as it is included in the third check (null instanceof SomeClass will always return false). This is just a performance issue, though, not a correctness one. However, you still need to provide a proper implementation of hashCode, otherwise the classes will fail to work when used in a HashMap. I'd suggest you let your IDE generate an implementation.

[oscargus]

My point was rather going from a broken equals to a slightly less broken equals (which is what happened here) rather than introducing a broken equals. Was the alternative of not touching the equals at all better? Simpler for sure.

I agree with you and, apart from the this == oher check is more or less how I usually write mine (no explicit checking for null).

Eclipse seems not to be one of those IDEs though. Or I haven't found the switch to make it more clever than super(other)...

[lenhard]

You would have gotten away without touching equals ;-) My point is: If you touch it, you should go all the way.

Eclipse generates you these methods via the menu: Source-> "Generate hashCode() and equals()". Your cursor needs to be in the class in which you want to generate the methods. In the following dialog, you can select the significant attributes of the class.

[oscargus]

Yes, that's my point. My question is if that is better? ;-)

Ahh, OK! Thanks! The generation based on warnings seems to be different.

[oscargus]

I added your bullets in the Wiki.

[lenhard]

Great! So this PR is now good to go, right? (at least from my point of view)

[Siedlerchr]

Be warned about the Eclipse generated equals methods, it is not always ideal/correct.
using instanceOf makes sense if you want subtypes to be equal. But that is a philsophocial questions...;)
And for HashCode there is the nice Object.hash() method.

[oscargus]

I merge this now. Do not think it can be much better. ;-)