[WFCORE-5611] Refactor Array copies (Controller) #4787
Conversation
github-actions
bot
added
the
deps-ok
| Set<String> set = new HashSet<String>(orderedChildTypes.length); | ||
| for (String type : orderedChildTypes) { | ||
| set.add(type); | ||
| Set<String> set = Collections.emptySet(); |
There was a problem hiding this comment.
Collections.emptySet() returns an immutable set, which makes to change the behavior of this method. We should avoid introducing these changes, the test suite passed and there are other methods that initialize this set as an immutable set, but if there is no argument against this request change, it would be preferable to keep the same behavior as before.
There was a problem hiding this comment.
@yersan Good point. I'll fix it.
| for (TimeUnit tu : allowed) { | ||
| allowedValues.add(tu); | ||
| if (allowed != null && allowed.length > 0) { | ||
| Collections.addAll(allowedValues, allowed); |
There was a problem hiding this comment.
Also, @boris-unckel just out of curiosity, what motivates you to replace the simple for loop by the Collections method?
It is inappreciable, so just to see your point of view, but these Collections.addAll() takes care of an internal flag to return whether all elements were added to the list. We do not need to take care of this flag here, so the previous for loop code is slightly faster than the one implemented on the Collections.addAll, so really I'm not sure how to justify these changes.
There was a problem hiding this comment.
@yersan I did not focus on the flag. Different to other refactorings this series "only" aims to use common (JDK) methods instead of custom code. If it's more like code beautifying we should skip the series.
There was a problem hiding this comment.
Hello @boris-unckel , if we finally conclude this is just a matter of code beautification, there are reasons to not include it, as you know, we backport changes to several other branches, and each commit slightly adds more difficulties to track the source of a change. These are quite weak arguments, but if we do not have anything against them I think would be better to not include them.
I've tried to also justify the changes based on the null checks, but even in that case, they sound a bit questionable to me too, for example, specifically to this class, throwing an NPE would make the developer use the correct available method, the one that does not use allowed as an argument, well, also a bit weak argument, but an argument, would you agree with it?
There was a problem hiding this comment.
Hi @yersan thanks for taking the time to weighten the pros/cons for this PR. I think the bottom line for this JDK util methods is that they are useful for new code, it increases reuse and readability. For existing code it's more code beautifying. I'm going to withdraw the series of PRs for this reason.
There was a problem hiding this comment.
You're welcome @boris-unckel , thanks to you for all the contributions you are doing to the project!
|
Close as "Won't do", see PR5611 |
Fixes https://issues.redhat.com/browse/WFCORE-5611
as part of https://issues.redhat.com/browse/WFCORE-5576