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

refactor ContextLoaderUtils class #173

Closed
wants to merge 1 commit into from

Conversation

ttddyy
Copy link
Contributor

@ttddyy ttddyy commented Oct 25, 2012

In ContextLoaderUtils class, these two methods contain similar loop logic

  • resolveContextLoaderClass(Class<?>, String)
  • resolveContextConfigurationAttributes(Class<?>)

Created a private method that returns a list of ContextConfigurationAttribute, and changed above two methods to use the new one to clean up the duplicated logic.

Further cleanup suggestion:

"resolveContextConfigurationAttributes(Class<?> clazz)" method returns "List< ContextConfigurationAttributes>", and the returned list has parent-class-first order.
On the other hand, the caller of the method, "buildMergedContextConfiguration(...)", uses the list in reverse order: child class first.
So, it would be easier to read if "resolveContextConfigurationAttributes" method returns the list as child-first order.
This method has unit tests in ContextLoaderUtilsTests, so they need to be updated as well.


Issue: SPR-9918

I have signed and agree to the terms of the SpringSource Individual
Contributor License Agreement.

@ghost ghost assigned sbrannen Oct 25, 2012
"resolveContextLoaderClass(Class<?>, String)" and
"resolveContextConfigurationAttributes(Class<?>)" methods
have similar loop logic.
Created a method that returns a list of ContextConfigurationAttribute,
and changed those two methods to use the new one to clean up the
duplicated loop.

Issue: SPR-9918
@sbrannen
Copy link
Member

Tadaya,

Thanks for pointing this out and submitting the pull request!

In order to simplify the code and remain consistent with the conventions currently in use in the class, I have chosen to address this issue differently than in your pull request. Please check out commit 33d5b01 for details.

Cheers,

Sam

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