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

improve FeatureRepository impl performance #96

Merged
merged 1 commit into from
Apr 18, 2019

Conversation

goekay
Copy link
Contributor

@goekay goekay commented Apr 16, 2019

the backing data structure for features was an ArrayList, which has O(n) worst-case
look-up performance. since the method findFeature(...) is frequently used, this has negative
implications on the performance of the library. A HashMap is better suited for feature look-up.

reason: the backing data structure for features was an ArrayList, which has O(n) worst-case
look-up performance. since the method findFeature(...) is frequently used, this has negative
implications on the performance of the library. A HashMap is better suited for feature look-up.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 43.472% when pulling d1a4a83 on goekay:master into 2ee40bc on ChargeTimeEU:master.

@codecov-io
Copy link

Codecov Report

Merging #96 into master will increase coverage by 0.3%.
The diff coverage is 71.42%.

@@             Coverage Diff             @@
##             master      #96     +/-   ##
===========================================
+ Coverage     41.87%   42.17%   +0.3%     
- Complexity      876      882      +6     
===========================================
  Files           214      214             
  Lines          4227     4225      -2     
  Branches        438      435      -3     
===========================================
+ Hits           1770     1782     +12     
+ Misses         2351     2337     -14     
  Partials        106      106

Copy link
Member

@TVolden TVolden left a comment

Choose a reason for hiding this comment

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

Thanks for your improvement, which makes good sense.

The code looks good. I only have one comment.

The tests could do with some better names to clearly state what they're testing and one assert per test. This will pay off when a failing test clearly states what it did and what it expected. I wrote this article about it https://github.com/ChargeTimeEU/Java-OCA-OCPP/wiki/Naming-unittest

@TVolden TVolden merged commit 3e61ee2 into ChargeTimeEU:master Apr 18, 2019
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.

4 participants