The reason this came up for me is using a different library (Commons DBCP)
that NPE'd on null inputs in a deep application stack, all the way from a
UI down to that library. My layer was in the middle. Granted I could have
added null checks in my code but it was cleaner to avoid the NPEs in DBCP.
I was just looking at Collections for something else and I noticed that the
same kind of problems would happen. Hence my interest in updating the code.
Gary
On Sun, May 20, 2018 at 5:18 PM, Dave Brosius <dbros...@mebigfatguy.com>
wrote:
> I'd be much more happy it the method was added, as something like
>
> safeAddAll
>
> so you knew when you were using it, but i agree then your api starts to
> bloat pretty badly.
>
>
> CollectionUtils.isEmpty checking for null makes sense. You are explicitly
> using the method to check for null as a condition.
>
>
> addAll doesn't say that to me when i use it. One argument for your
> proposal is that there really is no reason to use Collections.addAll
>
> over myRealCollection.addAll(x);, so perhaps adding this null check gives
> it value.
>
>
> Anyway, just my two cents. I have never used this method, so it doesn't
> effect me, so if in the end this is what you want to do, i guess it's ok,
> just showing the other side.
>
>
>
> On 05/20/2018 11:02 AM, Gary Gregory wrote:
>
>> Shall we do this in a more pragmatic fashion? One method at a time. Let's
>> start with changing:
>>
>> /**
>> * Adds all elements in the array to the given collection.
>> *
>> * @param <C> the type of object the {@link Collection} contains
>> * @param collection the collection to add to, must not be null
>> * @param elements the array of elements to add, must not be null
>> * @return {@code true} if the collection was changed, {@code false}
>> otherwise
>> * @throws NullPointerException if the collection or array is null
>> */
>> public static <C> boolean addAll(final Collection<C> collection,
>> final
>> C... elements) {
>> boolean changed = false;
>> for (final C element : elements) {
>> changed |= collection.add(element);
>> }
>> return changed;
>> }
>>
>> to:
>>
>> /**
>> * Adds all elements in the array to the given collection.
>> *
>> * @param <C> the type of object the {@link Collection} contains
>> * @param collection the collection to add to
>> * @param elements the array of elements to add
>> * @return {@code true} if the collection was changed, {@code false}
>> otherwise
>> */
>> public static <C> boolean addAll(final Collection<C> collection,
>> final
>> C... elements) {
>> boolean changed = false;
>> if (collection != null && elements != null) {
>> for (final C element : elements) {
>> changed |= collection.add(element);
>> }
>> }
>> return changed;
>> }
>>
>> Which makes the method not blow up when either inputs is null.
>>
>> Gary
>>
>> On Sat, May 19, 2018 at 10:15 PM, Dave Brosius <dbros...@mebigfatguy.com>
>> wrote:
>>
>> This protection concept sounds perfectly rational, until you think of
>>> other obvious things that do similar things to protect the coder.
>>>
>>> myLong.equals(myString) doesn't throw an exception for instance, it just
>>> return false.
>>>
>>> but then you never know that you have a problem in your code and just
>>> blindly go along using it seemingly working fine.
>>>
>>> Similarly, given that collection.addAll(null) throws, papering over that
>>> fact with CollectionUtils might do more harm than good.
>>>
>>>
>>>
>>>
>>> On 05/19/2018 09:10 PM, Gary Gregory wrote:
>>>
>>> On Sat, May 19, 2018 at 4:47 AM, sebb <seb...@gmail.com> wrote:
>>>>
>>>> On 18 May 2018 at 20:34, Gary Gregory <garydgreg...@gmail.com> wrote:
>>>>
>>>>> Hi All:
>>>>>>
>>>>>> A lot of methods in CollectionUtils are not null-safe and are
>>>>>> documented
>>>>>>
>>>>>> as
>>>>>
>>>>> such in Javadoc with throwing NPEs.
>>>>>>
>>>>>> I'd like to change that.
>>>>>>
>>>>>> To what?
>>>>>
>>>>> For example, this should not cause an NPE:
>>>>>
>>>> collectionA = new ArrayList();
>>>> CollectionUtils.addAll(collectionA, (Integer[]) null);
>>>>
>>>> Gary
>>>>
>>>> The change is behavioral and BC would be preserved.
>>>>
>>>>> Thoughts?
>>>>>>
>>>>>> It depends on the method whether a null parameter makes sense or not.
>>>>> In some cases it may be confusing if null is accepted.
>>>>> And what is meant by a null parameter.
>>>>>
>>>>> In any case, any such changes need a big warning in the release notes.
>>>>>
>>>>> Gary
>>>>> ---------------------------------------------------------------------
>>>>> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
>>>>> For additional commands, e-mail: dev-h...@commons.apache.org
>>>>>
>>>>>
>>>>>
>>>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
>>> For additional commands, e-mail: dev-h...@commons.apache.org
>>>
>>>
>>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> For additional commands, e-mail: dev-h...@commons.apache.org
>
>
