cognitect / transit-java

transit-format implementation for Java
Apache License 2.0
62 stars 21 forks source link

Override equals() and hashCode() in Impl-Classes #26

Open sne11ius opened 7 years ago

sne11ius commented 7 years ago

I think LinkImpl, RatioImpl etc. should override equals() and hashCode() more consistently.

My suggestions would be to generate all equals and hashCode implementations for these classes with your IDE of choice.

I would have created a pr, but I read you dont accept any...

puredanger commented 7 years ago

Comparing urls accurately (taking into account relative links, case insensitivity, etc) is notoriously difficult, so I'm not sure that LinkImpl should implement equals.

I think it would be a good idea to make equals and hashcode consistent for RatioImpl.

I may have some time Friday to look at this.

sne11ius commented 7 years ago

That was a fast answer, thank you :) With regard to LinkImpl: Your concerns are right from a "semantic" perspective I guess. However, I think a pure "technical" implementation (comparing all properties) would be just as reasonable - and certainly better than the current state.

puredanger commented 7 years ago

Well one of those properties is a URL and java.net.URL's equals() is famous for actually resolving host names into IP addresses, which is a blocking IO call. java.net.URI addresses some of these issues and would maybe have been a better choice here.

sne11ius commented 7 years ago

Wow, totally didn't know about the URL/equals issues - thanks again ;) So what about simply comparing the string representations of the urls? Do you think that could be viable approach?

puredanger commented 7 years ago

Might work - would probably be best to convert toURI(), then compare those for equality.

sne11ius commented 7 years ago

Sounds great, thank you!