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

Introduce PNC 2.0 Rest client #2

Merged
merged 74 commits into from
Apr 22, 2020
Merged

Conversation

jbartece
Copy link
Contributor

@jbartece jbartece commented Apr 1, 2020

No description provided.

@jbartece jbartece requested a review from dwalluck April 6, 2020 12:37
@jbartece
Copy link
Contributor Author

jbartece commented Apr 6, 2020

@dwalluck Hi, it is a good time for you to review my changed.
I have introduced PNC 2.0 client and did some refactoring, when doing the migration with attempts to reduce code complexity and make the maintenance easier.

I followed these steps:

  1. I studied the original code and tried to understand it
  2. I reduced the complexity of the find method by removing unused data from PNC
  3. I've added the new client and removed old
  4. Rewrote the whole code without caching using the new client
  5. Extracted the code to PncBuildFinder to reduce complexity and introduced BuildFinderUtils for the shared logic
  6. Performed other refactoring to reduce complexity of the BuildFinder

What's missing?

  1. I removed caching as the way it was implemented it caused very hardly readable code and also not good reusability of the caches with more instances of the BuildFinder. I'll introduce caching on the Client level, which should provide better performance gains with less complexity in the business logic
  2. Add integration tests and test everything. I did not test the code yet as I did many changes and rewrote some parts several times, but I'd like to have a review from you first :-)

I hope it won't be too big shock for you and you will like it :-)

@codecov-io
Copy link

codecov-io commented Apr 7, 2020

Codecov Report

Merging #2 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master       #2   +/-   ##
=======================================
  Coverage   48.76%   48.76%           
=======================================
  Files          35       35           
  Lines        2998     2998           
  Branches      365      365           
=======================================
  Hits         1462     1462           
  Misses       1379     1379           
  Partials      157      157           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0db5e8f...0db5e8f. Read the comment docs.

@jbartece
Copy link
Contributor Author

jbartece commented Apr 9, 2020

@dwalluck Both methods are used now.


private DummyPncClient dummyPncClient;

{
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is kind of strange to initialize here, no? I guess the only alternative is to make them static? JUnit has a @BeforeClass but it must be a static method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think, it doesn't matter in this case as it is a test class, which should have only one instance anyway. I did it this way for some reason, which I cannot recall right now :-D

@dwalluck dwalluck merged commit a321d4a into project-ncl:master Apr 22, 2020
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