git.net

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [1/2] ant git commit: Deprecate CollectionUtils and Enumerations; reduce explicit use of Enumeration


I accepted the original criticism that going Enumeration -> List -> Stream
has an overhead and I tried to address that by using a decorator.
I believe that Streams API can at least implement the logic run by an
original Enumeration in a more concise way, or provide more powerful idioms.
That IMO makes it worth the while to investigate the Streams alternatives.

In the process I also found out that other iterators could be used in a
more efficient way, or that there is a redundant object construction going
on.
If you wish, I may spend some time to describe the changes I introduced,
and maybe then we could discuss the their merits.

Gintas

2018-05-18 8:30 GMT+02:00 Jaikiran Pai <jai.forums2013@xxxxxxxxx>:

> If your objection is that I claimed that these qualify as "most of the
> cases" - I really don't know what else to say then. The original commit
> which did this change is this[1]. I haven't reviewed it fully, but the very
> first few changes that are done are these [2] [3] [4] [5][6].
>
> Of course, there's a subsequent commit which then uses a different new
> util, instead of just using the existing iterator/enumeration. Speaking of
> the subsequent commit, it still doesn't undo the (IMO unnecessary) change
> that was done to some of the code (take a look at
> ArgumentProcessorRegistry.java for example).
>
> Even if these don't fall under "most of the cases", why even change these
> places? I'm sure you would know this - the Enumeration or APIs that you
> refactored aren't even deprecated in Java version 10.
>
> Anyway, I'm really getting tired of these back and forth arguments on
> refactoring. The reason I get involved in certain open source projects is
> to get a chance to work with like minded developers and learn something out
> of it and not to go wage a war on which coding style is better or try and
> be critical of other committers' commits. Unfortunately, in the recent
> past, this has reached a state where I have ended up spending more time
> being critical of changes that have gone in, than actually adding much code
> of value. As much as I try to stay away from reviews or checking the commit
> logs, I just keep going back to them. I don't want to end up being a grumpy
> guy criticizing the commits. I'm just going to take a break from this for
> some days and be a regular user and come back and see if I still enjoy
> contributing.
>
> [1] https://github.com/apache/ant/commit/070c3bc86f85e8f01cb624f
> e50ae82f0d11171b2
>
> [2] https://github.com/apache/ant/commit/070c3bc86f85e8f01cb624f
> e50ae82f0d11171b2#diff-21eb59eaf9f2b5d0b487aeb5e5022ccdL746
> [3] https://github.com/apache/ant/commit/070c3bc86f85e8f01cb624f
> e50ae82f0d11171b2#diff-21eb59eaf9f2b5d0b487aeb5e5022ccdL834
> [4] https://github.com/apache/ant/commit/070c3bc86f85e8f01cb624f
> e50ae82f0d11171b2#diff-21eb59eaf9f2b5d0b487aeb5e5022ccdL888
> [5] https://github.com/apache/ant/commit/070c3bc86f85e8f01cb624f
> e50ae82f0d11171b2#diff-21eb59eaf9f2b5d0b487aeb5e5022ccdL1359
>
> [6] https://github.com/apache/ant/commit/070c3bc86f85e8f01cb624f
> e50ae82f0d11171b2#diff-b98a3d2097d6a9b5d7e0fc2eac033f24L348
>
>
> -Jaikiran
>
>
>
> On 18/05/18 11:15 AM, Gintautas Grigelionis wrote:
>
>> I'm not quite sure that what you say was true "in most of the cases".
>>
>> Gintas
>>
>> 2018-05-18 6:52 GMT+02:00 Jaikiran Pai <jai.forums2013@xxxxxxxxx>:
>>
>> To be honest, I don't think this deprecation/conversion change is
>>> good(including this recent commit whichintroduces a
>>> StreamUtils.enumerationAsStream(...))
>>>
>>> To give you an example, the code that was previously present would (in
>>> most of the cases) get hold of an enumeration and would iterate over it
>>> and
>>> during that iteration would runcertain logic. With the changes that went
>>> in
>>> (which again is a bulk change!) the code now gets hold of an enumeration
>>> and instead of just getting on the business of iterating and running
>>> certain logic, instead now passes this enumeration aroundto convert it
>>> into
>>> some other form (thus additional code plus additional objects allocated
>>> in
>>> the process), all this to iterate over it and run some logic on it - all
>>> of
>>> which was already possible with the enumeration that was already
>>> available.
>>>
>>>
>>> -Jaikiran
>>>
>>>
>>>
>>> On 18/05/18 12:22 AM, Gintautas Grigelionis wrote:
>>>
>>> Thanks for reviewing, I hope Spliterators will do a better job.
>>>>
>>>> Gintas
>>>>
>>>> 2018-05-17 8:37 GMT+02:00 Jaikiran Pai <jai.forums2013@xxxxxxxxx>:
>>>>
>>>> I agree. Especially when it's being done on something like for archive
>>>>
>>>>> entries which can betoo many depending on the archive that is being
>>>>> dealt
>>>>> with.
>>>>>
>>>>> -Jaikiran
>>>>>
>>>>>
>>>>>
>>>>> On 17/05/18 12:04 PM, Maarten Coene wrote:
>>>>>
>>>>> Converting an Enumeration to a List just for iterating it doesn't seem
>>>>>
>>>>>> performance and memory wise a good idea to me.
>>>>>> Maarten
>>>>>>
>>>>>>          Van: "gintas@xxxxxxxxxx" <gintas@xxxxxxxxxx>
>>>>>>     Aan: notifications@xxxxxxxxxxxxxx
>>>>>>     Verzonden: woensdag 16 mei 19:13 2018
>>>>>>     Onderwerp: [1/2] ant git commit: Deprecate CollectionUtils and
>>>>>> Enumerations; reduce explicit use of Enumeration
>>>>>>       Repository: ant
>>>>>> Updated Branches:
>>>>>>      refs/heads/master ac35c0014 -> 070c3bc86
>>>>>>
>>>>>>
>>>>>> http://git-wip-us.apache.org/repos/asf/ant/blob/070c3bc8/src
>>>>>> /main/org/apache/tools/ant/types/ZipScanner.java
>>>>>> ------------------------------------------------------------
>>>>>> ----------
>>>>>> diff --git a/src/main/org/apache/tools/ant/types/ZipScanner.java
>>>>>> b/src/main/org/apache/tools/ant/types/ZipScanner.java
>>>>>> index a3df040..5667159 100644
>>>>>> --- a/src/main/org/apache/tools/ant/types/ZipScanner.java
>>>>>> +++ b/src/main/org/apache/tools/ant/types/ZipScanner.java
>>>>>> @@ -20,7 +20,7 @@ package org.apache.tools.ant.types;
>>>>>>       import java.io.File;
>>>>>>     import java.io.IOException;
>>>>>> -import java.util.Enumeration;
>>>>>> +import java.util.Collections;
>>>>>>     import java.util.Map;
>>>>>>     import java.util.zip.ZipException;
>>>>>>     @@ -62,10 +62,7 @@ public class ZipScanner extends ArchiveScanner
>>>>>> {
>>>>>>                    "Only file provider resources are supported"));
>>>>>>              try (ZipFile zf = new ZipFile(srcFile, encoding)) {
>>>>>> -
>>>>>> -            Enumeration<ZipEntry> e = zf.getEntries();
>>>>>> -            while (e.hasMoreElements()) {
>>>>>> -                ZipEntry entry = e.nextElement();
>>>>>> +            for (ZipEntry entry : Collections.list(zf.getEntries()))
>>>>>> {
>>>>>>                    Resource r = new ZipResource(srcFile, encoding,
>>>>>> entry);
>>>>>>                    String name = entry.getName();
>>>>>>                    if (entry.isDirectory()) {
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> ---------------------------------------------------------------------
>>>>> To unsubscribe, e-mail: dev-unsubscribe@xxxxxxxxxxxxxx
>>>>> For additional commands, e-mail: dev-help@xxxxxxxxxxxxxx
>>>>>
>>>>>
>>>>>
>>>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@xxxxxxxxxxxxxx
>>> For additional commands, e-mail: dev-help@xxxxxxxxxxxxxx
>>>
>>>
>>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@xxxxxxxxxxxxxx
> For additional commands, e-mail: dev-help@xxxxxxxxxxxxxx
>
>