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

Change Equals() to only compare Value. #32

Closed
wants to merge 2 commits into from
Closed

Change Equals() to only compare Value. #32

wants to merge 2 commits into from

Conversation

aalmada
Copy link
Contributor

@aalmada aalmada commented Sep 30, 2018

This is a follow up on #30 that suggests changing Equals() and GetHashCode() to only compare Value. Advantages are explained in the issue description.

This PR also contains the following:

  • Adds TryFromName() and TryFromValue() methods. These allow searching without exceptions thrown, improving considerably performance.
  • Adds a ignoreCase parameter to FromName() and TryFromName(). The defaut value is trueto not break previous contract but, this is not the usual default value for this option and has considerably worst performance.
  • Adds unit test for equals operations.
  • Adds benchmarking project with option to benchmark equals or search operations.

Equals Benchmarks

screenshot at sep 30 21-18-34

The performance much better than for current master branch or even with changes proposed in #26 .

Search Benchmarks

screenshot at sep 30 21-27-07

The use os dictionaries make all search operations to have O(1) complexity. The use of exception is considerably slower than TryFromName() and TryFromValue() that are actually faster when value is invalid than when valid. Case sensitive searching is faster than when casing ignored.

…ng. Add unit tests for equals operations. Add benchmarking project.
@aalmada
Copy link
Contributor Author

aalmada commented Oct 1, 2018

The benchmarks of current master branch for reference:

screenshot at oct 01 08-50-44

screenshot at oct 01 08-53-56

@aalmada aalmada mentioned this pull request Oct 2, 2018
Copy link
Owner

@ardalis ardalis left a comment

Choose a reason for hiding this comment

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

Looks good - can you re-open?

@ardalis
Copy link
Owner

ardalis commented Nov 27, 2018

FYI, here's one place where I previously commented. @aalmada

Trying to find where you referenced the slides since I thought I replied there as well. In any case, the slide deck looked like a great presentation.

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

Successfully merging this pull request may close these issues.

None yet

2 participants