Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
sne11ius opened this issue Mar 15, 2017 · 6 comments
Open

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

sne11ius opened this issue Mar 15, 2017 · 6 comments

Comments

@sne11ius
Copy link

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

  • LinkImpl doesn't override any of the two
  • RatioImpl overrides equals(), but not hashCode(). And the implementation of equals() relies on object identity of the numerators und denominators

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

puredanger commented Mar 16, 2017

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
Copy link
Author

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
Copy link
Contributor

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

@sne11ius
Copy link
Author

Sounds great, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants