Awesome, thanks. I'm ok with this change:
- moduleLocations.removeIf(file -> !match((String) modules,
file));
-
+ for (final Iterator<File> i = moduleLocations.iterator();
i.hasNext(); ) {
+ final File file = i.next();
+ if (!match((String) modules, file)) {
+ i.remove();
+ }
+ }
I don't think anyone else has expressed an opinion one way or another, so
I'd leave your for-loop as it is.
Jon
On Mon, Dec 31, 2018 at 2:07 PM Gurkan Erdogdu <[email protected]> wrote:
> Hi Jon
> I will update my patch to update wildcard import, loop and diamond <>
> operator.
> Regards
> Gurkan
>
> On Mon, Dec 31, 2018 at 2:38 PM Jonathan Gallimore <
> [email protected]> wrote:
>
> > 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.
> >
> > I think like the other changes, and do think it looks neater and easier
> to
> > read.
> >
> > I personally prefer
> >
> > map = new HashMap<>();
> >
> > over
> >
> > map = new HashMap<Object, Object>();
> >
> > but I don't think that's a big deal.
> >
> > I also note this change:
> >
> > - moduleLocations.removeIf(file -> !match((String)
> modules,
> > file));
> > -
> > + for (final Iterator<File> i =
> moduleLocations.iterator();
> > i.hasNext(); ) {
> > + final File file = i.next();
> > + if (!match((String) modules, file)) {
> > + i.remove();
> > + }
> > + }
> >
> > Which is changing from a lambda to a loop. I'd probably have done it like
> > this, but the effect is the same:
> >
> > final Iterator<File> iterator =
> moduleLocations.iterator();
> > while (iterator.hasNext()) {
> > final File file = iterator.next();
> > if (!match((String) modules, file)) {
> > iterator.remove();
> > }
> > }
> >
> > IntelliJ is definitely preferring the moduleLocations.removeIf, and will
> > refactor both of those loops 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:
> >
> > 1. Backporting changes is much harder when you have to work around
> language
> > changes like this
> > 2. 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.
> >
> > Jon
> >
> > On Mon, Dec 31, 2018 at 10:21 AM Jonathan Gallimore <
> > [email protected]> wrote:
> >
> > > Yep, I'll take a look. Thanks for the PR.
> > >
> > > Jon
> > >
> > > On Mon, Dec 31, 2018 at 9:10 AM Gurkan Erdogdu <[email protected]>
> > > wrote:
> > >
> > >> Hi
> > >> I have created a PR (https://github.com/apache/tomee/pull/332) for
> > >> (TOMEE-2435)
> > >> Simplify the Code in org.apache.openejb.OpenEjbContainer$Provider
> > >> <https://issues.apache.org/jira/browse/TOMEE-2435>
> > >>
> > >> If anybody can check and merge, it is appreciated :)
> > >> Regards.
> > >> Gurkan
> > >>
> > >
> >
>
