[ 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: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]