[
https://issues.apache.org/jira/browse/COLLECTIONS-536?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Tagir Valeev updated COLLECTIONS-536:
-------------------------------------
Description:
In class org.apache.commons.collections(4).MapUtils there's a method
putAll(final Map<K, V> map, final Object[] array) which starts with
{noformat}
map.size(); // force NPE
{noformat}
Actually map.size() is not that harmless call for any map. For example,
consider java.util.concurrent.ConcurrentHashMap size() implementation:
depending on the circumstances it may take even more time than the rest of the
putAll method (at least prior to JDK 8). Things are even worse for
ConcurrentSkipListMap: its size() method iterates over all the map elements.
Thus I'd suggest to replace this call with more conventional check like:
{noformat}
if(map == null) {
throw new NullPointerException();
}
{noformat}
If you still want to save bytes, you may use {{map.getClass()}}. It's final, so
it will never be overridden to do something strange and it's even used by JavaC
for the same purpose (to trigger NPE). For example, if you compile and
disassemble this code:
{noformat}
public class Outer {
public class Inner {}
public void test(Outer n) { n.new Inner(); }
}
{noformat}
You will see a getClass() call which is used to trigger NPE.
was:
In class org.apache.commons.collections(4).MapUtils there's a method
putAll(final Map<K, V> map, final Object[] array) which starts with
map.size(); // force NPE
Actually map.size() is not that harmless call for any map. For example,
consider java.util.concurrent.ConcurrentHashMap size() implementation:
depending on the circumstances it may take even more time than the rest of the
putAll method (at least prior to JDK 8). I'd suggest to replace it with more
conventional check like:
if(map == null) {
throw new NullPointerException();
}
If you still want to save bytes, you may use map.getClass(). It's final, so it
will never be overridden to do something strange and it's even used by JavaC
for the same purpose (to trigger NPE). For example, if you compile and
disassemble this code:
public class Outer {
public class Inner {}
public void test(Outer n) { n.new Inner(); }
}
You will see a getClass() call which is used to trigger NPE.
> (Code style) map.size() call in MapUtils.putAll()
> -------------------------------------------------
>
> Key: COLLECTIONS-536
> URL: https://issues.apache.org/jira/browse/COLLECTIONS-536
> Project: Commons Collections
> Issue Type: Bug
> Components: Map
> Affects Versions: 3.2.1, 4.0
> Environment: Any
> Reporter: Tagir Valeev
> Priority: Trivial
> Labels: performance
>
> In class org.apache.commons.collections(4).MapUtils there's a method
> putAll(final Map<K, V> map, final Object[] array) which starts with
> {noformat}
> map.size(); // force NPE
> {noformat}
> Actually map.size() is not that harmless call for any map. For example,
> consider java.util.concurrent.ConcurrentHashMap size() implementation:
> depending on the circumstances it may take even more time than the rest of
> the putAll method (at least prior to JDK 8). Things are even worse for
> ConcurrentSkipListMap: its size() method iterates over all the map elements.
> Thus I'd suggest to replace this call with more conventional check like:
> {noformat}
> if(map == null) {
> throw new NullPointerException();
> }
> {noformat}
> If you still want to save bytes, you may use {{map.getClass()}}. It's final,
> so it will never be overridden to do something strange and it's even used by
> JavaC for the same purpose (to trigger NPE). For example, if you compile and
> disassemble this code:
> {noformat}
> public class Outer {
> public class Inner {}
> public void test(Outer n) { n.new Inner(); }
> }
> {noformat}
> You will see a getClass() call which is used to trigger NPE.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
