commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Niall Pemberton (JIRA)" <j...@apache.org>
Subject [jira] Resolved: (BEANUTILS-267) BeanComparator(String, Comparator) should check the comparator for null and default to ComparableComparator.getInstance()
Date Sat, 20 Jan 2007 11:54:29 GMT

     [ https://issues.apache.org/jira/browse/BEANUTILS-267?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]

Niall Pemberton resolved BEANUTILS-267.
---------------------------------------

       Resolution: Fixed
    Fix Version/s: 1.8.0
         Assignee: Niall Pemberton

Fixed, thanks for the suggestion:

http://svn.apache.org/viewvc?view=rev&revision=498105

> BeanComparator(String, Comparator) should check the comparator for null and default to
ComparableComparator.getInstance()
> -------------------------------------------------------------------------------------------------------------------------
>
>                 Key: BEANUTILS-267
>                 URL: https://issues.apache.org/jira/browse/BEANUTILS-267
>             Project: Commons BeanUtils
>          Issue Type: Improvement
>          Components: Bean-Collections
>    Affects Versions: 1.7.0
>            Reporter: Jacob Kjome
>         Assigned To: Niall Pemberton
>            Priority: Minor
>             Fix For: 1.8.0
>
>
> The way the BeanComparator(String, Comparator) constructor is implemented is inconvenient.
 I've got code that passes in a comparator.  This comparator may be null.  I assumed that
the 2-args constructor would sanely ignore a null comparator argument and use a default like
ReverseComparator does in commons-collections, but alas, no.  I have to do the null check
before I pass it in  
> For instance, here's the constructor for ReverseComparator, which takes a Comparator
argument...
>     public ReverseComparator(Comparator comparator) {
>         if(comparator != null) {
>             this.comparator = comparator;
>         } else {
>             this.comparator = ComparableComparator.getInstance();
>         }
>     }
> The null check and provided default is convenient and reasonable.  
> Here's the current constructor for BeanComparator that can only end in a NullPointerException
if provided  null comparator....
>     public BeanComparator( String property, Comparator comparator ) {
>         setProperty( property );
>         this.comparator = comparator;
>     }
> Why not?....
>     public BeanComparator( String property, Comparator comparator ) {
>         setProperty( property );
>         if(comparator != null) {
>             this.comparator = comparator;
>         } else {
>             this.comparator = ComparableComparator.getInstance();
>         }
>     }
> The fact that BeanComparator allows itself to be put in a bad state by storing a null
comparator which it later tries to use with no null check, guaranteeing a NullPointerException,
probably should be considered a bug.  However, since it works just fine when provided a non-null
comparator, I consider this more of an "Improvement" opportunity than a bug, thus the reported
Issue Type.  Hopefully this can be applied to the next release.
> Jake

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators: https://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org


Mime
View raw message