commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bruno P. Kinoshita (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (MATH-1408) Do not use exceptions for control flow
Date Wed, 05 Apr 2017 08:19:41 GMT

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

Bruno P. Kinoshita edited comment on MATH-1408 at 4/5/17 8:19 AM:
------------------------------------------------------------------

Hummm. Started working on the pull request, first identifying where CCE was being used. But
then realised it may not be that simple. Let's take Frequency as example.

It has no generic types, but creates an internal TreeMap, with keys with the type `Comparator<?>`.
Then whenever you add or get values from the TreeMap, it will throw CCE if you pass a key
value with a different type from the others stored in the map.

{code}
Frequency f = new Frequency();
// Okay
f.addValue("Ola");
// Key 12, type Integer, does not match String type. CCE, wrapped in a Math exception
f.addValue(12);
{code}

And even a simpler example without Math classes to help outlining the issue.

{code}
TreeMap<Integer, String> map = new TreeMap<>();
int key = 1;
map.put(key, "One");
// Okay
map.get(100);
// TreeMap will try to call 1.1#compareTo against each Key it contains, which can result
// in a CCE here.
map.get(1.1);
{code}

It is not clear to me how we could compare the key type to the given v value. Checking if
the map is not empty, and then checking the type of the first key doesn't sound very elegant
(and not sure if it would be a valid solution). I guess an option to eliminate the CCE could
be generify the Frequency class maybe (though I didn't spend much time checking if that would
work well), but also not sure if that would work for the other cases of CCE.

Any ideas?


was (Author: kinow):
Hummm. Started working on the pull request, first identifying where CCE was being used. But
then realised it may not be that simple. Let's take Frequency as example.

It has no generic types, but creates an internal TreeMap, with keys with the type `Comparator<?>`.
Then whenever you add or get values from the TreeMap, it will throw CCE if you pass a key
value with a different type from the others stored in the map.

{code}
Frequency f = new Frequency();
// Okay
f.addValue("Ola");
// Key 12, type Integer, does not match String type. CCE, wrapped in a Math exception
f.addValue(12);
{code}

And even a simpler example without Math classes to help outlining the issue.

{code}
TreeMap<Integer, String> map = new 
int key = 1;
map.put(key, "One");
// Okay
map.get(100);
// TreeMap will try to call 1.1#compareTo against each Key it contains, which can result
// in a CCE here.
map.get(1.1);
{code}

It is not clear to me how we could compare the key type to the given v value. Checking if
the map is not empty, and then checking the type of the first key doesn't sound very elegant
(and not sure if it would be a valid solution). I guess an option to eliminate the CCE could
be generify the Frequency class maybe (though I didn't spend much time checking if that would
work well), but also not sure if that would work for the other cases of CCE.

Any ideas?

> Do not use exceptions for control flow
> --------------------------------------
>
>                 Key: MATH-1408
>                 URL: https://issues.apache.org/jira/browse/MATH-1408
>             Project: Commons Math
>          Issue Type: Task
>            Reporter: Gilles
>            Priority: Minor
>              Labels: control, exception, flow
>             Fix For: 4.0
>
>
> There are several occurrences where exception is used to control flow.
> Code such as
> {noformat}
> try  {
>  // block A
> } catch (ClassCastException e) {
>  // block B
> }
> {noformat}
> where "block A" is trying to cast an object "o" to "SomeClass", should be changed to
> {noformat}
> if (o instanceof SomeClass)  {
>  // block A
> } else {
>  // block B
> }
> {noformat}



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

Mime
View raw message