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

enhancement: use constructor injection instead of field injection #1839

Merged
merged 3 commits into from
Jan 13, 2019
Merged

enhancement: use constructor injection instead of field injection #1839

merged 3 commits into from
Jan 13, 2019

Conversation

kezhenxu94
Copy link
Member

@kezhenxu94 kezhenxu94 commented Jan 5, 2019

This patch uses Constructor Injection to avoid the drawbacks of Field Injection. What's more, Spring Documentation also recommends using Constructor Injection here

Here are some of the drawbacks of Field Injection listed in this article:

  • Disallows immutable field declaration

  • Eases single responsibility principle violation

  • Tightly coupled with dependency injection container

  • Hidden dependencies

@codecov-io
Copy link

codecov-io commented Jan 5, 2019

Codecov Report

Merging #1839 into master will increase coverage by 1.55%.
The diff coverage is 95.2%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1839      +/-   ##
============================================
+ Coverage     48.31%   49.87%   +1.55%     
- Complexity     1913     1914       +1     
============================================
  Files           394      394              
  Lines         11680    12075     +395     
  Branches       1225     1225              
============================================
+ Hits           5643     6022     +379     
- Misses         5590     5606      +16     
  Partials        447      447
Impacted Files Coverage Δ Complexity Δ
.../apollo/portal/service/PortalDBPropertySource.java 29.41% <ø> (ø) 3 <0> (ø) ⬇️
...k/apollo/portal/component/RestTemplateFactory.java 100% <ø> (ø) 5 <0> (ø) ⬇️
...k/apollo/common/datasource/TitanEntityManager.java 0% <0%> (ø) 0 <0> (ø) ⬇️
.../apollo/portal/spi/ctrip/BizLoggingCustomizer.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...ollo/portal/spi/ctrip/WebContextConfiguration.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...lo/portal/spi/configuration/AuthConfiguration.java 6.09% <0%> (-0.32%) 1 <0> (ø)
...ork/apollo/biz/customize/BizLoggingCustomizer.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...rk/apollo/biz/eureka/ApolloEurekaClientConfig.java 66.66% <100%> (+16.66%) 2 <1> (ø) ⬇️
...ework/apollo/portal/controller/ItemController.java 7.14% <100%> (+5.62%) 1 <1> (ø) ⬇️
.../apollo/portal/listener/ConfigPublishListener.java 21.81% <100%> (+15.29%) 2 <1> (ø) ⬇️
... and 115 more

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 1dc461f...360f995. Read the comment docs.

@coveralls
Copy link

coveralls commented Jan 5, 2019

Coverage Status

Coverage increased (+1.4%) to 53.573% when pulling 61819dc on kezhenxu94:style/use-constructor-injection into 5d15562 on ctripcorp:master.

@kezhenxu94
Copy link
Member Author

There are some Field Injections left and some workarounds such as @Lazy annotation, but it needs refactoring if we want to replace them with Constructor Injection, and I'll do that if it's acceptable to you @nobodyiam , if any further changes should be done, please let me know. Thanks

@nobodyiam
Copy link
Member

@kezhenxu94 Thanks!

I can see using constructor injection makes the program more robust and more friendly to tests. However, I'm curious about how did you manage to modify so many places, by hand or by some tool?

I also notice there are some places marked with @Lazy and some places not, is there any simple rule to distinguish the different usage?

@kezhenxu94
Copy link
Member Author

kezhenxu94 commented Jan 7, 2019

I can see using constructor injection makes the program more robust and more friendly to tests. However, I'm curious about how did you manage to modify so many places, by hand or by some tool?

Modifying those places with modern IDE is a easy task, I'm using Intellij.

I also notice there are some places marked with @Lazy and some places not, is there any simple rule to distinguish the different usage?

The rule is that when the dependencies form a circle, i.e. A depends on B and B depends on A, then the dependencies must be annotated as @Lazy or exceptions will be thrown, this is considered a bad practice and need refactoring

@nobodyiam
Copy link
Member

@kezhenxu94

Thanks for the explanation. Regarding the cyclic reference, how did you identify such case, manually or by some tool?

@kezhenxu94
Copy link
Member Author

@kezhenxu94

Thanks for the explanation. Regarding the cyclic reference, how did you identify such case, manually or by some tool?

It's hard to identify that, though there are tools to detect that in this thread, I identified by running the test cases and Spring will print out the cyclic denpendencies graph.

@nobodyiam
Copy link
Member

@kezhenxu94

Thanks! Will take a detailed look soon!

Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

LGTM

@nobodyiam nobodyiam merged commit d927552 into apolloconfig:master Jan 13, 2019
CrackerCat pushed a commit to CrackerCat/apollo-1 that referenced this pull request Jul 31, 2024
…olloconfig#1839)

* enhancement: use constructor injection instead of field injection

* remove unused field
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