[jira] Resolved: (BEANUTILS-267) BeanComparator(String, Comparator) should check the comparator for null and default to ComparableComparator.getInstance()

["Niall Pemberton (JIRA)" <ji...@apache.org>]

     [ 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