(TOMEE-2435) Simplify the Code in org.apache.openejb.OpenEjbContainer#332
(TOMEE-2435) Simplify the Code in org.apache.openejb.OpenEjbContainer#332gerdogdu wants to merge 2 commits intoapache:masterfrom gerdogdu:master
Conversation
| if (map == null) { // JBoss EJB API pass null when calling EJBContainer.createEJBContainer() | ||
| map = new HashMap<>(); | ||
| if (map == null) { | ||
| map = new HashMap<Object, Object>(); |
There was a problem hiding this comment.
why did you remove the diamond operator?
brunobat
left a comment
brunobat
left a comment
There was a problem hiding this comment.
On a General note.
This code you are touching is used by everything and is rarely touched. You are introducing extensive changes. Prudence advises to also add new tests to make sure the changes are ok.
| if (map == null) { // JBoss EJB API pass null when calling EJBContainer.createEJBContainer() | ||
| map = new HashMap<>(); | ||
| if (map == null) { | ||
| map = new HashMap<Object, Object>(); |
There was a problem hiding this comment.
We should use the diamond operator only.
| // ignore | ||
| }finally{ | ||
| if(field != null){ | ||
| field.setAccessible(accessible); |
There was a problem hiding this comment.
Won't this change the behaviour? What will be the consequences?
There was a problem hiding this comment.
This code you are touching is used by everything and is rarely touched. You are introducing extensive changes. Prudence advises to also add new tests to make sure the changes are ok.
I did not change the logic of the class, just separated the big method to smaller parts for the purpose of readable and understandable code.
The idea is that TomEE community accept that the code is indeed unreadable and then can accept my patch and then if there needs to have any further changes, we can continue to patch or update.
I think that this is a normal procedure for working with community. This is a simple change. For example, if you want to look for an extensive change, it is a microprofile addition to the core TomEE codebase :)
There was a problem hiding this comment.
ok Gurkan.
So, ok means will you commit?
There was a problem hiding this comment.
@gerdogdu There's a couple of comments for you on the dev@ mailing list:
Is there any particular reason for flipping the imports to wildcards? We tend to import the individual classes elsewhere in the codebase. My personal preference would be to leave those as individual imports. It doesn't make the import list that much longer, and I find its clearer what is being imported and used. IDEs tend to fold those away unless you're specifically looking at them too.
IntelliJ is definitely preferring the Java 8 style moduleLocations.removeIf, and will refactor the loop to it with one keystroke. I have the what is probably an unpopular opinion which is that I actually prefer the loop over the lambda. The reasons are:
- Backporting changes is much harder when you have to work around language changes like this
- I personally find lots of things crammed into one line harder to read, and in this particular case, there's a cast and a negation (so a double negative) in the removeIf.
I am currently running tests against your PR and master, along with code coverage. Although I appreciate you have extracted methods to make this more readable and not changed functionality, it is a core class, and I think running those checks is a reasonable thing to do.
Generally speaking, I think this looks ok, and if you can reply to those couple of points on the list, I think we can get this merged in.
Thanks again for taking the time to create the PR.
There was a problem hiding this comment.
Thanks for the comments. I will update my patch and attach again.
Regards.
Gurkan
|
I will close this pull request and create another. |
|
Happy for you to do that, but your last push to this branch looked fine to merge. |
4 participants
(TOMEE-2435) Simplify the Code in org.apache.openejb.OpenEjbContainer