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

Issue 172: optimize map hasKey() and hasEntry() #292

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

brownian-motion
Copy link

Implements the enhancement in #172

Instead of a linear search over all entries, this change leverages Map.containsKey(K key) to check that a map hasKey() or hasEntry().

Copy link

@nibsi nibsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that the changes regarding visibility overshoot the scope of the issue, and warrant their own issue and pull request.

@brownian-motion
Copy link
Author

Thank you for your feedback, @nibsi . I'm afraid I created the branch from my local master branch, which included a different change, instead of origin/master; I'm embarrassed I missed that before pushing. I'll rebase and push a PR without the unrelated change.

Copy link

@nibsi nibsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to add a unit test that checks that you can use the hasKey() and hasEntry() matchers with null keys even if the map implementation doesn't allow null keys, if it isn't there already. Otherwise, it looks good to me.

@brownian-motion
Copy link
Author

brownian-motion commented Apr 16, 2020

I've added some tests that cover the relevant edge cases covered by the code addition. Thank you very much for your help, @nibsi! I believe this commit is ready for final review + merging if approved.

@tumbarumba , would you or a colleague in this repo please consider reviewing this PR when convenient?

@sf105
Copy link
Member

sf105 commented Apr 19, 2020

Interesting question, the null thing is not something I thought of. The question is what is the right behaviour when using null with an inappropriate collection. Personally, I would have expected it to fail, rather than mismatch, as that's what would happen in the production code. I fear that we'll be hiding failure modes.

Also, is that a copy'n'paste in the comment?

@brownian-motion
Copy link
Author

brownian-motion commented Apr 20, 2020

@sf105 There's one case I can think of where the safe API is better: asserting that a null key is absent:

assertThat(new Hashtable<Object, Object>(), not(hasKey(null)));

I would expect this to pass and continue to the next assertion; allowing a null pointer exception would force you to avoid touching particular types/code paths or start to put conditional logic and try/catches in your tests. Since it seems reasonable to assert that "this map type which prohibits null keys does not contain a null key", I support keeping this API that allows expressing that.

Also, sorry, where do you mean? I'm not sure what copy-and-paste you mean.

@brownian-motion
Copy link
Author

@sf105 I would be happy to make whichever further changes you think would benefit the project the best. When convenient, please let me know what further adjustments might be appropriate here.

@nhojpatrick
Copy link
Member

@brownian-motion looks good, please can you rebase from master, as hamcrest-core and hamcrest-library have been refactored a lot and also deprecated, so that everything is just in hamcrest.

…taining.hasEntry(),

which use the map's own containsKey(K key) method to avoid an O(n) worst-case linear search for every match.

Incorporated null-safety checks to account for maps that do not support null keys, per @nibsi 's recommendation
@brownian-motion
Copy link
Author

Hello again! I know it's been two years, but I've rebased this PR and would be happy to help with any further steps to get it ready for merging.

@nhojpatrick please feel free to look through this again when convenient :)

@nhojpatrick
Copy link
Member

Going to try and kick start hamcrest, so if you want to get it merged, please rebase from the branch v2.3-candidates.
Still trying to understand how has permissions to perform a release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Needs triage
Development

Successfully merging this pull request may close these issues.

4 participants