commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sebb (JIRA)" <j...@apache.org>
Subject [jira] Commented: (MATH-259) Bugs in Frequency API
Date Fri, 17 Apr 2009 13:56:14 GMT

    [ https://issues.apache.org/jira/browse/MATH-259?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12700183#action_12700183
] 

Sebb commented on MATH-259:
---------------------------

See:

URL: http://svn.apache.org/viewvc?rev=765996&view=rev
Log:
MATH-259 - check for Comparable when adding values

I overlooked the change of Exception, so there's also:

URL: http://svn.apache.org/viewvc?rev=766003&view=rev
Log:
MATH-259 - throw IllegalArgument rather than ClassCast to better retain original behaviour

==

I added a new method

   public void addValue(Comparable<?> v)

which is called from

   public void addValue(Object v)

which I took the liberty of deprecating, so the compiler will warn users about non-Comparable
objects.
Hope that's OK.

Note that it's still possible for mutually non-Comparable values to be added, because the
code does not check comparisons both ways, it relies on HashMap to do so.

I.e. if B.compareTo(A) is OK, but A.compareTo(B) does not exist, then it is possible to add
A, then B.
This causes a ClassCastException in some of the getXXX() methods.

> Bugs in Frequency API
> ---------------------
>
>                 Key: MATH-259
>                 URL: https://issues.apache.org/jira/browse/MATH-259
>             Project: Commons Math
>          Issue Type: Bug
>            Reporter: Sebb
>
> I think the existing Frequency API has some bugs in it.
> The addValue(Object v) method allows one to add a plain Object, but one cannot add anything
further to the instance, as the second add fails with IllegalArgumentException.
> In fact, the problem is with the first call to addValue(Object) which should not allow
a plain Object to be added - it should only allow Comparable objects.
> This could be fixed by checking that the object is Comparable.
> Similar considerations apply to the getCumFreq(Object) and getCumPct(Object) methods
- they will only work with objects that implement Comparable.
> The getCount(Object) and getPct(Object) methods don't fail when given a non-Comparable
object (because the class cast exception is caught), however they just return 0 as if the
object was not present:
> {code}
>         final Object OBJ = new Object();
>         f.addValue(OBJ); // This ought to fail, but doesn't, causing the unexpected behaviour
below
>         System.out.println(f.getCount(OBJ)); // 0
>         System.out.println(f.getPct(OBJ)); // 0.0
> {code}
> Rather than adding extra checks for Comparable, it seems to me that the API would be
much improved by using Comparable instead of Object.
> Also, it should make it easier to implement generics.
> However, this would cause compilation failures for some programs that pass Object rather
than Comparable to the class.
> These would need recoding, but I think they would continue to run OK against the new
API.
> It would also affect the run-time behaviour slightly, as the first attempt to add a non-Comparable
object would fail, rather than the second add of a possibly valid object.
> But is that a viable program? It can only add one object, and any attempt to get statistics
will either return 0 or an Exception, and applying the instanceof fix would also cause it
to fail.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message