Overriding `hashCode` method when overriding `equals` method

[Neutius]

As of pull request #45 the class org.locationtech.proj4j.proj.Projection and ten of its subclasses override the equals method that is inherited from the Object class.

However, the hashCode method is not overridden, and I'm pretty sure that's considered bad practice. This is described in more detail by Joshua Bloch in Item 11 of his book Effective Java, but it boils down to this: if two objects are considered equal by the equals method, they should also return the same value when their respective hashCode method is called.

Overriding equals but not hashCode violates the contract for the hashCode method, and makes instances of this class unusable for collections such as HashMap and HashSet.

[Neutius]

I made an attempt to generate a hashCode override for the Projection class, but I simply lack the domain/GIS/mathematical knowledge to know what I'm doing.

I did notice the current equals override doesn't check for the name field, which might be an oversight. There are about a dozen other fields that may or not be relevant for overriding equals and hashCode, I have no way of telling.

[pomadchin]

@Neutius it doesn't compare by the name intentionally, read through this PR description. It can be different for the same projection depending on how the instance of the projection was created.

P.S. There is a useful class we can probably use to simplify the code to write hashCode function overloads.

[Neutius]

@pomadchin I didn't notice the name was not used intententionally, thanks for pointing that out.

Using that class directly would add a dependency to this project, which I would like to avoid. I usually use IntelliJ to generate equals and hashCode overrides, but for a class this complicated it results in something like this: https://github.com/Neutius/proj4j/commit/887753c9c412cb6d64b8a93fc3cd2109e9ee9c7e

I'm pretty sure we could find a way to make readable and maintainable code without adding a dependency.

[pomadchin]

@Neutius ah yeah, I didn’t mean to add an extra dependency, but to use this class as an example of how we could make a more consistent hash code generation.

[Neutius]

I fiddled around a bit, still not happy with it, but at least it's something: https://github.com/Neutius/proj4j/commit/c3f48a0312180fa82c9d8a372d20b5c2c448d176

[metasim]

Is it a fair statement to say that the state used to compute hashCode should be exactly the same state used to compute equals? If so, I'd argue hashCode should be implemented in the concrete specializations rather than the base class, side-by-side with equals.

As a snarky aside, the fact that these classes are mutable makes me shudder to think that anyone would ever put them in a Hash{Set|Table} etc.

[Neutius]

I fully agree with you on the first part. In my attempt, I included every field included in the equals override. We should absolutely override hashCode in the ten subclasses that override equals. Most subclasses don't, I guess the override in the Projection base class is a good fit for those?

I could generate and refactor hashCode overrides for those ten subclasses, but again, I lack the domain/GIS/mathematical knowledge to understand the contents of what I'm doing. I could probably deliver some decent Java code, though.

Edit: my co-worker pointed out I should use the Objects.hash method, which results in something like this: https://github.com/Neutius/proj4j/commit/0679ed05be0385cc50d7f54245fa2357cfd32a0d

I used all the field in the equals override, and in the same order, to increase maintainability. Should I do the same for the 10 subclasses that override equals?

[metasim]

I used all the field in the equals override, and in the same order, to increase maintainability. Should I do the same for the 10 subclasses that override equals?

That would be my assumption, but I defer to the others here.

I wonder if we should consider using this or something similar?

https://jqno.nl/equalsverifier/

[pomadchin]

@metasim dunno; I will look into it, thanks for the link. I think it is a critical issue now, I will have to look into it.