git.net

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

Re: [collections] Null-safe addAll(final Collection<C> collection, final C... elements) [WAS] CollectionUtils not null-safe


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 <dbrosius@xxxxxxxxxxxxxxx>
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 <dbrosius@xxxxxxxxxxxxxxx>
>> 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 <sebbaz@xxxxxxxxx> wrote:
>>>>
>>>> On 18 May 2018 at 20:34, Gary Gregory <garydgregory@xxxxxxxxx> 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-unsubscribe@xxxxxxxxxxxxxxxxxx
>>>>> For additional commands, e-mail: dev-help@xxxxxxxxxxxxxxxxxx
>>>>>
>>>>>
>>>>>
>>>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@xxxxxxxxxxxxxxxxxx
>>> For additional commands, e-mail: dev-help@xxxxxxxxxxxxxxxxxx
>>>
>>>
>>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@xxxxxxxxxxxxxxxxxx
> For additional commands, e-mail: dev-help@xxxxxxxxxxxxxxxxxx
>
>