You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Gilles Sadowski <gi...@harfang.homelinux.org> on 2011/05/01 01:33:23 UTC
Re: [math] Restoring IAE to MathUtils#binomialCoefficient methods
On Sat, Apr 30, 2011 at 09:10:08AM -0700, Phil Steitz wrote:
> Converting some of my code to use trunk, I discovered that the
> binomialCoefficient methods no longer throw IllegalArgumentException
> when parameters are invalid.
The consensus was a singly rooted hierarchy ("MathRuntimeException").
The advantage being commonly agreed on was to offer the "map" functionality
for adding messages and context information.
> The javadoc asserts that
> MathIllegalArgumentException will be thrown in these cases, but that
> is not correct,
I don't understand; the code for "checkBinomial" can throw
"NumberIsTooLargeException" or "NotPositiveException" and they are
subclasses of "MathIllegalArgumentException".
> since what is actually thrown now can differ
> depending on the parameter problem
That's a feature, naturally: Different problem, different exception.
> and the resulting exceptions are
> neither standard IAEs nor descendents of MathIAE.
>From what I see, they are descendents of MathIAE.
> I have patched
> the code to return a standard IAE (with localized message). Per
> discussion in [1] it looks like we were close to consensus to favor
> standard exceptions and in this case,
No, that thread was discussing
throwing standard "NullPointerException"
vs
throwing a CM-specific "NullArgumentException" (subtype of MathIAE)
vs
not checking for null pointer at all.
[I don't think that a consensus has been reached on that issue.]
For all the other cases of invalid parameters passed to methods, it had
been settled already that "MathIllegalArgumentException" (or subclasses
thereof) would been thrown.
> I would much rather return a
> standard IAE with meaningful error message rather than a
> non-standard RTE (with exactly the same error message and generally
> confusing type - e.g. "NumberIsTooSmall" when n, k parameters are
> not in the right order) and keep the javadoc simple. Otherwise, the
> main method javadoc has to be rewritten to conform to what the code
> now does.
The Javadoc "@throws" tag is not incorrect:
-----
* @throws MathIllegalArgumentException if preconditions are not met
-----
But it is not as precise as it could (by mentioning the types actually
thrown by "checkBinomial").
The main description is indeed a remnant of the old behaviour and it is yet
another proof that it is not good documentation practise to repeat the
(supposedly) same information several times.
Documentation for methods "binomialCoefficientDouble" and
"binomialCoefficientLog" also refer to the old behaviour and must be
updated.
> Are there any objections to this change?
Unless I didn't understand the problem, yes.
-1 to revert to a mix of standard and CM-specific exceptions.
Gilles
> Phil
>
> [1] *http://s.apache.org/nr*
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] Restoring IAE to MathUtils#binomialCoefficient methods
Posted by Phil Steitz <ph...@gmail.com>.
On 5/2/11 1:38 AM, Gilles Sadowski wrote:
> On Sun, May 01, 2011 at 03:18:20PM -0700, Phil Steitz wrote:
>> On 5/1/11 2:29 PM, Gilles Sadowski wrote:
>>> On Sun, May 01, 2011 at 08:11:00AM -0700, Phil Steitz wrote:
>>>> On 5/1/11 3:48 AM, Gilles Sadowski wrote:
>>>>> On Sat, Apr 30, 2011 at 10:53:30PM -0700, Phil Steitz wrote:
>>>>>> On 4/30/11 4:33 PM, Gilles Sadowski wrote:
>>>>>>> On Sat, Apr 30, 2011 at 09:10:08AM -0700, Phil Steitz wrote:
>>>>>>>> Converting some of my code to use trunk, I discovered that the
>>>>>>>> binomialCoefficient methods no longer throw IllegalArgumentException
>>>>>>>> when parameters are invalid.
>>>>>>> The consensus was a singly rooted hierarchy ("MathRuntimeException").
>>>>>>> The advantage being commonly agreed on was to offer the "map" functionality
>>>>>>> for adding messages and context information.
>>>>>> I guess I misunderstood and after really seeing the consequences in
>>>>>> my own code, I am going to have to ask that we reopen that
>>>>>> discussion - i.e., I would like us to throw IAE and other standard
>>>>>> exceptions where appropriate, as in this case, as we have up to and
>>>>>> through 2.2. I know I said before that I did not see this as worth
>>>>>> arguing about, but I really think this change is a bad API design
>>>>>> decision that will cause needless hassle and surprising RTEs for
>>>>>> users upgrading to the new version.
>>>>> I'm astonished, and for the time being, will refrain from other comments.
>>>>>
>>>>>>>> The javadoc asserts that
>>>>>>>> MathIllegalArgumentException will be thrown in these cases, but that
>>>>>>>> is not correct,
>>>>>>> I don't understand; the code for "checkBinomial" can throw
>>>>>>> "NumberIsTooLargeException" or "NotPositiveException" and they are
>>>>>>> subclasses of "MathIllegalArgumentException".
>>>>>>>
>>>>>> Sorry. my mistake.
>>>>>>>> since what is actually thrown now can differ
>>>>>>>> depending on the parameter problem
>>>>>>> That's a feature, naturally: Different problem, different exception.
>>>>>>>
>>>>>> That's where I disagree. I see zero value added and in fact
>>>>>> confusing complexity introduced by these exceptions. When you ask
>>>>>> for B(n,k) where k is not less than or equal to n, a standard IAE
>>>>>> with a message that says precisely that (which the current message
>>>>>> does) is clear and *better* that a "NumberIsTooLargeException".
>>>>>> What number? I guess it must be k? To figure it out you have to
>>>>>> look at the exception message, which is *exactly the same message*
>>>>>> that the old code reported. If we really think we need to
>>>>>> specialize and report different exceptions for every precondition
>>>>>> violation (which I do not agree with), then these exceptions should
>>>>>> be meaningful in the context of the API that is using them. So
>>>>>> here, for example, we would have to throw something like
>>>>>> "CombinationSizeTooLargeForSetException."
>>>>> Then, we do _not_ disagree _now_. From the start, I stated that a consistent
>>>>> design would be to define a specific exception for each specific that must
>>>>> be reported, especially if it must contain complex functionality like
>>>>> localization.
>>>>> IIRC, either you or Luc (or both) did not want a large number of exceptions.
>>>>> To keep the number down, we reuse less specific exception types (like
>>>>> "NumberIsTooLargeException" in several contexts) and rely on the message(s)
>>>>> for context information. Nothing lost from the previous situation (when one
>>>>> *had* to rely solely on the message)!
>>>> But in this case, my opinion is that IAE is just fine - there is
>>>> nothing more "specific" to communicate unless you want to define
>>>> something meaningful in the context of the API. "NumberIsTooLarge"
>>>> has no value here.
>>> I don't agree. This is the concept that describes the problem: indeed, the
>>> precondition is that "k" must be smaller than "n".
>>> This has the same value as the "out of range" concept where you give the
>>> value of some parameter and the values of the bounds.
>>>
>> No, it is actually a different concept. In mathematical terms, it
>> is a binary relation that is failing here: k < n. What
>> "NumberIsTooLarge" (or "NumberIsTooSmall" which could also be
>> applied here, to n instead of k) conveys is unary.
> I repeat that I'd be fine with adding an exception that would retain
> the binary relationship. If you think it is overkill, fine too. But that
> does not make "NumberIsTooLargeException" useless; it is only not precise
> enough in this case to fully describe the mathematical situation. From a
> programming viewpoint, it is IMO a good compromise: It says something about
> how the precondition was tested:
> ---
> if (k > n) {
> throw new NumberIsTooLargeException(k, n);
> }
> ---
>
>>>> As I said, if we feel we need to make this
>>>> particular exception due to precondition violation more precise, we
>>>> would need to define an exception that refers to subset relation
>>>> size or somesuch, which I don't see as necessary or valuable.
>>> In principle, I've nothing against devising a deeper hierarchy that
>>> would make the type of the exception refer to the exact problem. However,
>>> there would indeed be not much practical value added if all use cases are
>>> about letting the exception bubble upwards until it aborts the program (at
>>> which point a human will read the error mesage).
>> Sounds like you are arguing here to just leave it as IAE, which is
>> fine by me.
> No, see below.
>
>>>>> To answer your question above: No, you don't have to "guess" which number is
>>>>> too large; there is an accessor "getArgument()" that returns the number that
>>>>> triggered the exception and another "getMax()" that informs you about the
>>>>> maximum allowed number.
>>>> No, all that is reported is the *value*.
>>> Well, of course: CM is a numerical library, not a symbolic one.
>>>
>>>> To make this actually
>>>> work, you would have to do something like store and report the
>>>> formal parameter name. I don't see the point in this in general
>>> Me neither.
>>>
>> Then I would claim "NumberIsTooLarge" and "NumberIsTooSmall" provide
>> no value. You need to look at the exception message, which
>> thankfully we have preserved in this case, to understand what the
>> actual problem is. The abstraction itself is worthless at least in
>> this case, IMO.
>>
>>>> and
>>>> certainly not in this case, as what is problematic is the order
>>>> relation among the two parameters - not one is "too small" or the
>>>> other is "too large" but that they violate the stated preconditions
>>>> of the method.
>>> I really don't understand what bothers you!
>>> The precondition
>>> k <= n
>>> means that if k > n, then the *value* of k is too large.
>>> You can elaborate on the context if you wish, but that does not change the
>>> basic problem.
>> You are missing the point, stated above. The problem is that we
>> have a precondition that is a *relation* between the two parameters
>> and that precondition has been violated. Throwing in a
>> context-unaware unary predicate that says "one of the parameters is
>> too large" adds nothing to precise the problem.
> No, in my view, you are missing my many points.
> I understand your point about binary relation but, I repeat, CM is not aimed
> to be a faithful representation of all mathematical concepts; it is aimed at
> solving problems numerically. And to aid debugging when something goes wrong,
> Java provides a mechanism called "exceptions".
> Concerning the abstraction "number is too large", I find it useful just for
> that. Exactly as "out of range" gives a value plus two other values, claiming
> that the first one is not in the range defined by the other two.
>
> If you want that CM throws subtypes of IAE when a precondition fails, that's
> fine with me. It's not fine with me to throw a plain IAE because
> (1) that is not what we agreed on earlier to mitigate the ugliness (in my
> view) of having localization an integral part of CM,
This has nothing to do with messages. The "NumberIsTooLarge" thing
still carries *exactly the same message* which is what is really
relevant to the caller.
> (2) it will not provide the "rich context" feature,
Look carefully at how the code now appears *from the standpoint of
the caller* reading the javadoc. There is nothing useful added as
the exception conveys nothing meaningful beyond IAE.
> (3) it does not provide the flexibility that an application programmer might
> want for customizing an error message.
Don't follow this.
In any case, I give up. I can see we are getting nowhere here.
Phil
>
> Gilles
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] Restoring IAE to MathUtils#binomialCoefficient methods
Posted by Gilles Sadowski <gi...@harfang.homelinux.org>.
On Sun, May 01, 2011 at 03:18:20PM -0700, Phil Steitz wrote:
> On 5/1/11 2:29 PM, Gilles Sadowski wrote:
> > On Sun, May 01, 2011 at 08:11:00AM -0700, Phil Steitz wrote:
> >> On 5/1/11 3:48 AM, Gilles Sadowski wrote:
> >>> On Sat, Apr 30, 2011 at 10:53:30PM -0700, Phil Steitz wrote:
> >>>> On 4/30/11 4:33 PM, Gilles Sadowski wrote:
> >>>>> On Sat, Apr 30, 2011 at 09:10:08AM -0700, Phil Steitz wrote:
> >>>>>> Converting some of my code to use trunk, I discovered that the
> >>>>>> binomialCoefficient methods no longer throw IllegalArgumentException
> >>>>>> when parameters are invalid.
> >>>>> The consensus was a singly rooted hierarchy ("MathRuntimeException").
> >>>>> The advantage being commonly agreed on was to offer the "map" functionality
> >>>>> for adding messages and context information.
> >>>> I guess I misunderstood and after really seeing the consequences in
> >>>> my own code, I am going to have to ask that we reopen that
> >>>> discussion - i.e., I would like us to throw IAE and other standard
> >>>> exceptions where appropriate, as in this case, as we have up to and
> >>>> through 2.2. I know I said before that I did not see this as worth
> >>>> arguing about, but I really think this change is a bad API design
> >>>> decision that will cause needless hassle and surprising RTEs for
> >>>> users upgrading to the new version.
> >>> I'm astonished, and for the time being, will refrain from other comments.
> >>>
> >>>>>> The javadoc asserts that
> >>>>>> MathIllegalArgumentException will be thrown in these cases, but that
> >>>>>> is not correct,
> >>>>> I don't understand; the code for "checkBinomial" can throw
> >>>>> "NumberIsTooLargeException" or "NotPositiveException" and they are
> >>>>> subclasses of "MathIllegalArgumentException".
> >>>>>
> >>>> Sorry. my mistake.
> >>>>>> since what is actually thrown now can differ
> >>>>>> depending on the parameter problem
> >>>>> That's a feature, naturally: Different problem, different exception.
> >>>>>
> >>>> That's where I disagree. I see zero value added and in fact
> >>>> confusing complexity introduced by these exceptions. When you ask
> >>>> for B(n,k) where k is not less than or equal to n, a standard IAE
> >>>> with a message that says precisely that (which the current message
> >>>> does) is clear and *better* that a "NumberIsTooLargeException".
> >>>> What number? I guess it must be k? To figure it out you have to
> >>>> look at the exception message, which is *exactly the same message*
> >>>> that the old code reported. If we really think we need to
> >>>> specialize and report different exceptions for every precondition
> >>>> violation (which I do not agree with), then these exceptions should
> >>>> be meaningful in the context of the API that is using them. So
> >>>> here, for example, we would have to throw something like
> >>>> "CombinationSizeTooLargeForSetException."
> >>> Then, we do _not_ disagree _now_. From the start, I stated that a consistent
> >>> design would be to define a specific exception for each specific that must
> >>> be reported, especially if it must contain complex functionality like
> >>> localization.
> >>> IIRC, either you or Luc (or both) did not want a large number of exceptions.
> >>> To keep the number down, we reuse less specific exception types (like
> >>> "NumberIsTooLargeException" in several contexts) and rely on the message(s)
> >>> for context information. Nothing lost from the previous situation (when one
> >>> *had* to rely solely on the message)!
> >> But in this case, my opinion is that IAE is just fine - there is
> >> nothing more "specific" to communicate unless you want to define
> >> something meaningful in the context of the API. "NumberIsTooLarge"
> >> has no value here.
> > I don't agree. This is the concept that describes the problem: indeed, the
> > precondition is that "k" must be smaller than "n".
> > This has the same value as the "out of range" concept where you give the
> > value of some parameter and the values of the bounds.
> >
>
> No, it is actually a different concept. In mathematical terms, it
> is a binary relation that is failing here: k < n. What
> "NumberIsTooLarge" (or "NumberIsTooSmall" which could also be
> applied here, to n instead of k) conveys is unary.
I repeat that I'd be fine with adding an exception that would retain
the binary relationship. If you think it is overkill, fine too. But that
does not make "NumberIsTooLargeException" useless; it is only not precise
enough in this case to fully describe the mathematical situation. From a
programming viewpoint, it is IMO a good compromise: It says something about
how the precondition was tested:
---
if (k > n) {
throw new NumberIsTooLargeException(k, n);
}
---
> >> As I said, if we feel we need to make this
> >> particular exception due to precondition violation more precise, we
> >> would need to define an exception that refers to subset relation
> >> size or somesuch, which I don't see as necessary or valuable.
> > In principle, I've nothing against devising a deeper hierarchy that
> > would make the type of the exception refer to the exact problem. However,
> > there would indeed be not much practical value added if all use cases are
> > about letting the exception bubble upwards until it aborts the program (at
> > which point a human will read the error mesage).
>
> Sounds like you are arguing here to just leave it as IAE, which is
> fine by me.
No, see below.
> >>> To answer your question above: No, you don't have to "guess" which number is
> >>> too large; there is an accessor "getArgument()" that returns the number that
> >>> triggered the exception and another "getMax()" that informs you about the
> >>> maximum allowed number.
> >> No, all that is reported is the *value*.
> > Well, of course: CM is a numerical library, not a symbolic one.
> >
> >> To make this actually
> >> work, you would have to do something like store and report the
> >> formal parameter name. I don't see the point in this in general
> > Me neither.
> >
>
> Then I would claim "NumberIsTooLarge" and "NumberIsTooSmall" provide
> no value. You need to look at the exception message, which
> thankfully we have preserved in this case, to understand what the
> actual problem is. The abstraction itself is worthless at least in
> this case, IMO.
>
> >> and
> >> certainly not in this case, as what is problematic is the order
> >> relation among the two parameters - not one is "too small" or the
> >> other is "too large" but that they violate the stated preconditions
> >> of the method.
> > I really don't understand what bothers you!
> > The precondition
> > k <= n
> > means that if k > n, then the *value* of k is too large.
> > You can elaborate on the context if you wish, but that does not change the
> > basic problem.
>
> You are missing the point, stated above. The problem is that we
> have a precondition that is a *relation* between the two parameters
> and that precondition has been violated. Throwing in a
> context-unaware unary predicate that says "one of the parameters is
> too large" adds nothing to precise the problem.
No, in my view, you are missing my many points.
I understand your point about binary relation but, I repeat, CM is not aimed
to be a faithful representation of all mathematical concepts; it is aimed at
solving problems numerically. And to aid debugging when something goes wrong,
Java provides a mechanism called "exceptions".
Concerning the abstraction "number is too large", I find it useful just for
that. Exactly as "out of range" gives a value plus two other values, claiming
that the first one is not in the range defined by the other two.
If you want that CM throws subtypes of IAE when a precondition fails, that's
fine with me. It's not fine with me to throw a plain IAE because
(1) that is not what we agreed on earlier to mitigate the ugliness (in my
view) of having localization an integral part of CM,
(2) it will not provide the "rich context" feature,
(3) it does not provide the flexibility that an application programmer might
want for customizing an error message.
Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] Restoring IAE to MathUtils#binomialCoefficient methods
Posted by Phil Steitz <ph...@gmail.com>.
On 5/1/11 2:29 PM, Gilles Sadowski wrote:
> On Sun, May 01, 2011 at 08:11:00AM -0700, Phil Steitz wrote:
>> On 5/1/11 3:48 AM, Gilles Sadowski wrote:
>>> On Sat, Apr 30, 2011 at 10:53:30PM -0700, Phil Steitz wrote:
>>>> On 4/30/11 4:33 PM, Gilles Sadowski wrote:
>>>>> On Sat, Apr 30, 2011 at 09:10:08AM -0700, Phil Steitz wrote:
>>>>>> Converting some of my code to use trunk, I discovered that the
>>>>>> binomialCoefficient methods no longer throw IllegalArgumentException
>>>>>> when parameters are invalid.
>>>>> The consensus was a singly rooted hierarchy ("MathRuntimeException").
>>>>> The advantage being commonly agreed on was to offer the "map" functionality
>>>>> for adding messages and context information.
>>>> I guess I misunderstood and after really seeing the consequences in
>>>> my own code, I am going to have to ask that we reopen that
>>>> discussion - i.e., I would like us to throw IAE and other standard
>>>> exceptions where appropriate, as in this case, as we have up to and
>>>> through 2.2. I know I said before that I did not see this as worth
>>>> arguing about, but I really think this change is a bad API design
>>>> decision that will cause needless hassle and surprising RTEs for
>>>> users upgrading to the new version.
>>> I'm astonished, and for the time being, will refrain from other comments.
>>>
>>>>>> The javadoc asserts that
>>>>>> MathIllegalArgumentException will be thrown in these cases, but that
>>>>>> is not correct,
>>>>> I don't understand; the code for "checkBinomial" can throw
>>>>> "NumberIsTooLargeException" or "NotPositiveException" and they are
>>>>> subclasses of "MathIllegalArgumentException".
>>>>>
>>>> Sorry. my mistake.
>>>>>> since what is actually thrown now can differ
>>>>>> depending on the parameter problem
>>>>> That's a feature, naturally: Different problem, different exception.
>>>>>
>>>> That's where I disagree. I see zero value added and in fact
>>>> confusing complexity introduced by these exceptions. When you ask
>>>> for B(n,k) where k is not less than or equal to n, a standard IAE
>>>> with a message that says precisely that (which the current message
>>>> does) is clear and *better* that a "NumberIsTooLargeException".
>>>> What number? I guess it must be k? To figure it out you have to
>>>> look at the exception message, which is *exactly the same message*
>>>> that the old code reported. If we really think we need to
>>>> specialize and report different exceptions for every precondition
>>>> violation (which I do not agree with), then these exceptions should
>>>> be meaningful in the context of the API that is using them. So
>>>> here, for example, we would have to throw something like
>>>> "CombinationSizeTooLargeForSetException."
>>> Then, we do _not_ disagree _now_. From the start, I stated that a consistent
>>> design would be to define a specific exception for each specific that must
>>> be reported, especially if it must contain complex functionality like
>>> localization.
>>> IIRC, either you or Luc (or both) did not want a large number of exceptions.
>>> To keep the number down, we reuse less specific exception types (like
>>> "NumberIsTooLargeException" in several contexts) and rely on the message(s)
>>> for context information. Nothing lost from the previous situation (when one
>>> *had* to rely solely on the message)!
>> But in this case, my opinion is that IAE is just fine - there is
>> nothing more "specific" to communicate unless you want to define
>> something meaningful in the context of the API. "NumberIsTooLarge"
>> has no value here.
> I don't agree. This is the concept that describes the problem: indeed, the
> precondition is that "k" must be smaller than "n".
> This has the same value as the "out of range" concept where you give the
> value of some parameter and the values of the bounds.
>
No, it is actually a different concept. In mathematical terms, it
is a binary relation that is failing here: k < n. What
"NumberIsTooLarge" (or "NumberIsTooSmall" which could also be
applied here, to n instead of k) conveys is unary.
>> As I said, if we feel we need to make this
>> particular exception due to precondition violation more precise, we
>> would need to define an exception that refers to subset relation
>> size or somesuch, which I don't see as necessary or valuable.
> In principle, I've nothing against devising a deeper hierarchy that
> would make the type of the exception refer to the exact problem. However,
> there would indeed be not much practical value added if all use cases are
> about letting the exception bubble upwards until it aborts the program (at
> which point a human will read the error mesage).
Sounds like you are arguing here to just leave it as IAE, which is
fine by me.
>>> To answer your question above: No, you don't have to "guess" which number is
>>> too large; there is an accessor "getArgument()" that returns the number that
>>> triggered the exception and another "getMax()" that informs you about the
>>> maximum allowed number.
>> No, all that is reported is the *value*.
> Well, of course: CM is a numerical library, not a symbolic one.
>
>> To make this actually
>> work, you would have to do something like store and report the
>> formal parameter name. I don't see the point in this in general
> Me neither.
>
Then I would claim "NumberIsTooLarge" and "NumberIsTooSmall" provide
no value. You need to look at the exception message, which
thankfully we have preserved in this case, to understand what the
actual problem is. The abstraction itself is worthless at least in
this case, IMO.
>> and
>> certainly not in this case, as what is problematic is the order
>> relation among the two parameters - not one is "too small" or the
>> other is "too large" but that they violate the stated preconditions
>> of the method.
> I really don't understand what bothers you!
> The precondition
> k <= n
> means that if k > n, then the *value* of k is too large.
> You can elaborate on the context if you wish, but that does not change the
> basic problem.
You are missing the point, stated above. The problem is that we
have a precondition that is a *relation* between the two parameters
and that precondition has been violated. Throwing in a
context-unaware unary predicate that says "one of the parameters is
too large" adds nothing to precise the problem.
Phil
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] Restoring IAE to MathUtils#binomialCoefficient methods
Posted by Gilles Sadowski <gi...@harfang.homelinux.org>.
On Sun, May 01, 2011 at 08:11:00AM -0700, Phil Steitz wrote:
> On 5/1/11 3:48 AM, Gilles Sadowski wrote:
> > On Sat, Apr 30, 2011 at 10:53:30PM -0700, Phil Steitz wrote:
> >> On 4/30/11 4:33 PM, Gilles Sadowski wrote:
> >>> On Sat, Apr 30, 2011 at 09:10:08AM -0700, Phil Steitz wrote:
> >>>> Converting some of my code to use trunk, I discovered that the
> >>>> binomialCoefficient methods no longer throw IllegalArgumentException
> >>>> when parameters are invalid.
> >>> The consensus was a singly rooted hierarchy ("MathRuntimeException").
> >>> The advantage being commonly agreed on was to offer the "map" functionality
> >>> for adding messages and context information.
> >> I guess I misunderstood and after really seeing the consequences in
> >> my own code, I am going to have to ask that we reopen that
> >> discussion - i.e., I would like us to throw IAE and other standard
> >> exceptions where appropriate, as in this case, as we have up to and
> >> through 2.2. I know I said before that I did not see this as worth
> >> arguing about, but I really think this change is a bad API design
> >> decision that will cause needless hassle and surprising RTEs for
> >> users upgrading to the new version.
> > I'm astonished, and for the time being, will refrain from other comments.
> >
> >>>> The javadoc asserts that
> >>>> MathIllegalArgumentException will be thrown in these cases, but that
> >>>> is not correct,
> >>> I don't understand; the code for "checkBinomial" can throw
> >>> "NumberIsTooLargeException" or "NotPositiveException" and they are
> >>> subclasses of "MathIllegalArgumentException".
> >>>
> >> Sorry. my mistake.
> >>>> since what is actually thrown now can differ
> >>>> depending on the parameter problem
> >>> That's a feature, naturally: Different problem, different exception.
> >>>
> >> That's where I disagree. I see zero value added and in fact
> >> confusing complexity introduced by these exceptions. When you ask
> >> for B(n,k) where k is not less than or equal to n, a standard IAE
> >> with a message that says precisely that (which the current message
> >> does) is clear and *better* that a "NumberIsTooLargeException".
> >> What number? I guess it must be k? To figure it out you have to
> >> look at the exception message, which is *exactly the same message*
> >> that the old code reported. If we really think we need to
> >> specialize and report different exceptions for every precondition
> >> violation (which I do not agree with), then these exceptions should
> >> be meaningful in the context of the API that is using them. So
> >> here, for example, we would have to throw something like
> >> "CombinationSizeTooLargeForSetException."
> > Then, we do _not_ disagree _now_. From the start, I stated that a consistent
> > design would be to define a specific exception for each specific that must
> > be reported, especially if it must contain complex functionality like
> > localization.
> > IIRC, either you or Luc (or both) did not want a large number of exceptions.
> > To keep the number down, we reuse less specific exception types (like
> > "NumberIsTooLargeException" in several contexts) and rely on the message(s)
> > for context information. Nothing lost from the previous situation (when one
> > *had* to rely solely on the message)!
> But in this case, my opinion is that IAE is just fine - there is
> nothing more "specific" to communicate unless you want to define
> something meaningful in the context of the API. "NumberIsTooLarge"
> has no value here.
I don't agree. This is the concept that describes the problem: indeed, the
precondition is that "k" must be smaller than "n".
This has the same value as the "out of range" concept where you give the
value of some parameter and the values of the bounds.
> As I said, if we feel we need to make this
> particular exception due to precondition violation more precise, we
> would need to define an exception that refers to subset relation
> size or somesuch, which I don't see as necessary or valuable.
In principle, I've nothing against devising a deeper hierarchy that
would make the type of the exception refer to the exact problem. However,
there would indeed be not much practical value added if all use cases are
about letting the exception bubble upwards until it aborts the program (at
which point a human will read the error mesage).
> > To answer your question above: No, you don't have to "guess" which number is
> > too large; there is an accessor "getArgument()" that returns the number that
> > triggered the exception and another "getMax()" that informs you about the
> > maximum allowed number.
> No, all that is reported is the *value*.
Well, of course: CM is a numerical library, not a symbolic one.
> To make this actually
> work, you would have to do something like store and report the
> formal parameter name. I don't see the point in this in general
Me neither.
> and
> certainly not in this case, as what is problematic is the order
> relation among the two parameters - not one is "too small" or the
> other is "too large" but that they violate the stated preconditions
> of the method.
I really don't understand what bothers you!
The precondition
k <= n
means that if k > n, then the *value* of k is too large.
You can elaborate on the context if you wish, but that does not change the
basic problem.
> >>>> and the resulting exceptions are
> >>>> neither standard IAEs nor descendents of MathIAE.
> >>> >From what I see, they are descendents of MathIAE.
> >>>
> >>>> I have patched
> >>>> the code to return a standard IAE (with localized message). Per
> >>>> discussion in [1] it looks like we were close to consensus to favor
> >>>> standard exceptions and in this case,
> >>> No, that thread was discussing
> >>> throwing standard "NullPointerException"
> >>> vs
> >>> throwing a CM-specific "NullArgumentException" (subtype of MathIAE)
> >>> vs
> >>> not checking for null pointer at all.
> >>> [I don't think that a consensus has been reached on that issue.]
> >>>
> >>> For all the other cases of invalid parameters passed to methods, it had
> >>> been settled already that "MathIllegalArgumentException" (or subclasses
> >>> thereof) would been thrown.
> >>>
> >>>> I would much rather return a
> >>>> standard IAE with meaningful error message rather than a
> >>>> non-standard RTE (with exactly the same error message and generally
> >>>> confusing type - e.g. "NumberIsTooSmall" when n, k parameters are
> >>>> not in the right order) and keep the javadoc simple. Otherwise, the
> >>>> main method javadoc has to be rewritten to conform to what the code
> >>>> now does.
> >>> The Javadoc "@throws" tag is not incorrect:
> >>> -----
> >>> * @throws MathIllegalArgumentException if preconditions are not met
> >>> -----
> >>> But it is not as precise as it could (by mentioning the types actually
> >>> thrown by "checkBinomial").
> >>> The main description is indeed a remnant of the old behaviour and it is yet
> >>> another proof that it is not good documentation practise to repeat the
> >>> (supposedly) same information several times.
> >>> Documentation for methods "binomialCoefficientDouble" and
> >>> "binomialCoefficientLog" also refer to the old behaviour and must be
> >>> updated.
> >> Regardless of how we settle this, we *must* keep the javadoc
> >> consistent with what the code does and we *must* document fully both
> >> preconditions and exceptions thrown.
> > Certainly, please open a JIRA ticket for this specific issue.
> >
> >
> > Gilles
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] Restoring IAE to MathUtils#binomialCoefficient methods
Posted by Phil Steitz <ph...@gmail.com>.
On 5/1/11 3:48 AM, Gilles Sadowski wrote:
> On Sat, Apr 30, 2011 at 10:53:30PM -0700, Phil Steitz wrote:
>> On 4/30/11 4:33 PM, Gilles Sadowski wrote:
>>> On Sat, Apr 30, 2011 at 09:10:08AM -0700, Phil Steitz wrote:
>>>> Converting some of my code to use trunk, I discovered that the
>>>> binomialCoefficient methods no longer throw IllegalArgumentException
>>>> when parameters are invalid.
>>> The consensus was a singly rooted hierarchy ("MathRuntimeException").
>>> The advantage being commonly agreed on was to offer the "map" functionality
>>> for adding messages and context information.
>> I guess I misunderstood and after really seeing the consequences in
>> my own code, I am going to have to ask that we reopen that
>> discussion - i.e., I would like us to throw IAE and other standard
>> exceptions where appropriate, as in this case, as we have up to and
>> through 2.2. I know I said before that I did not see this as worth
>> arguing about, but I really think this change is a bad API design
>> decision that will cause needless hassle and surprising RTEs for
>> users upgrading to the new version.
> I'm astonished, and for the time being, will refrain from other comments.
>
>>>> The javadoc asserts that
>>>> MathIllegalArgumentException will be thrown in these cases, but that
>>>> is not correct,
>>> I don't understand; the code for "checkBinomial" can throw
>>> "NumberIsTooLargeException" or "NotPositiveException" and they are
>>> subclasses of "MathIllegalArgumentException".
>>>
>> Sorry. my mistake.
>>>> since what is actually thrown now can differ
>>>> depending on the parameter problem
>>> That's a feature, naturally: Different problem, different exception.
>>>
>> That's where I disagree. I see zero value added and in fact
>> confusing complexity introduced by these exceptions. When you ask
>> for B(n,k) where k is not less than or equal to n, a standard IAE
>> with a message that says precisely that (which the current message
>> does) is clear and *better* that a "NumberIsTooLargeException".
>> What number? I guess it must be k? To figure it out you have to
>> look at the exception message, which is *exactly the same message*
>> that the old code reported. If we really think we need to
>> specialize and report different exceptions for every precondition
>> violation (which I do not agree with), then these exceptions should
>> be meaningful in the context of the API that is using them. So
>> here, for example, we would have to throw something like
>> "CombinationSizeTooLargeForSetException."
> Then, we do _not_ disagree _now_. From the start, I stated that a consistent
> design would be to define a specific exception for each specific that must
> be reported, especially if it must contain complex functionality like
> localization.
> IIRC, either you or Luc (or both) did not want a large number of exceptions.
> To keep the number down, we reuse less specific exception types (like
> "NumberIsTooLargeException" in several contexts) and rely on the message(s)
> for context information. Nothing lost from the previous situation (when one
> *had* to rely solely on the message)!
But in this case, my opinion is that IAE is just fine - there is
nothing more "specific" to communicate unless you want to define
something meaningful in the context of the API. "NumberIsTooLarge"
has no value here. As I said, if we feel we need to make this
particular exception due to precondition violation more precise, we
would need to define an exception that refers to subset relation
size or somesuch, which I don't see as necessary or valuable.
> To answer your question above: No, you don't have to "guess" which number is
> too large; there is an accessor "getArgument()" that returns the number that
> triggered the exception and another "getMax()" that informs you about the
> maximum allowed number.
No, all that is reported is the *value*. To make this actually
work, you would have to do something like store and report the
formal parameter name. I don't see the point in this in general and
certainly not in this case, as what is problematic is the order
relation among the two parameters - not one is "too small" or the
other is "too large" but that they violate the stated preconditions
of the method.
>>>> and the resulting exceptions are
>>>> neither standard IAEs nor descendents of MathIAE.
>>> >From what I see, they are descendents of MathIAE.
>>>
>>>> I have patched
>>>> the code to return a standard IAE (with localized message). Per
>>>> discussion in [1] it looks like we were close to consensus to favor
>>>> standard exceptions and in this case,
>>> No, that thread was discussing
>>> throwing standard "NullPointerException"
>>> vs
>>> throwing a CM-specific "NullArgumentException" (subtype of MathIAE)
>>> vs
>>> not checking for null pointer at all.
>>> [I don't think that a consensus has been reached on that issue.]
>>>
>>> For all the other cases of invalid parameters passed to methods, it had
>>> been settled already that "MathIllegalArgumentException" (or subclasses
>>> thereof) would been thrown.
>>>
>>>> I would much rather return a
>>>> standard IAE with meaningful error message rather than a
>>>> non-standard RTE (with exactly the same error message and generally
>>>> confusing type - e.g. "NumberIsTooSmall" when n, k parameters are
>>>> not in the right order) and keep the javadoc simple. Otherwise, the
>>>> main method javadoc has to be rewritten to conform to what the code
>>>> now does.
>>> The Javadoc "@throws" tag is not incorrect:
>>> -----
>>> * @throws MathIllegalArgumentException if preconditions are not met
>>> -----
>>> But it is not as precise as it could (by mentioning the types actually
>>> thrown by "checkBinomial").
>>> The main description is indeed a remnant of the old behaviour and it is yet
>>> another proof that it is not good documentation practise to repeat the
>>> (supposedly) same information several times.
>>> Documentation for methods "binomialCoefficientDouble" and
>>> "binomialCoefficientLog" also refer to the old behaviour and must be
>>> updated.
>> Regardless of how we settle this, we *must* keep the javadoc
>> consistent with what the code does and we *must* document fully both
>> preconditions and exceptions thrown.
> Certainly, please open a JIRA ticket for this specific issue.
>
>
> Gilles
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] Restoring IAE to MathUtils#binomialCoefficient methods
Posted by Luc Maisonobe <Lu...@free.fr>.
Le 03/05/2011 12:03, Gilles Sadowski a écrit :
> On Tue, May 03, 2011 at 01:08:29AM +0200, Gilles Sadowski wrote:
>>>> [...]
>>>>> Consistency implies that *all* exceptions thrown from CM must behave the
>>>>> same way. I thus propose to add an interface like (maybe a better name?):
>>>>> ---
>>>>> interface ContextedException {
>>>>> void addMessage(Localizable pattern,
>>>>> Object ... arguments);
>>>>> void setContext(String key, Object value);
>>>>> Object getContext(String key);
>>>>> Set<String> getContextKeys();
>>>>> String getMessage(final Locale locale);
>>>>> String getMessage(final Locale locale,
>>>>> final String separator);
>>>>> }
>>>>> And all CM exceptions will implement this interface. [Instead of
>>>>> automatically inheriting the behaviour by being subclasses of
>>>>> "MathRuntimeException".]
>>>>>
>>>> I would prefer as stated above to revert to actual RTEs per 2.x
>>>> behavior. Above would be an improvement, as at least the unexpected
>>>> RTEs at upgrade would not bite (as they did me), but I see no reason
>>>> to add this machinery which is no less complex than what we had in
>>>> 2.x. Lets see what others think.
>>>
>>> Do the above mean we would have:
>>>
>>> public class MathIllegalArgumentException
>>> extends IllegalArgumentException
>>> implements ContextedException
>>
>> Yes. [Not sure about the name "ContextedException" yet.]
>>
>>> If so, then I am OK with this.
>
> I've just been thinking that, as per the concerns raised in the thread about
> interface overuse, it would be better to create a "thin" interface:
> ---
> public interface ExceptionContextProvider {
> /**
> * @return the context data structure that enables the customization of
> * the error message.
> */
> ExceptionContext getContext();
> }
> ---
>
> Then, all CM exceptions would be like
> ---
> public class MathIllegalArgumentException extends IllegalArgumentException
> implements ExceptionContextProvider {
> // ...
> }
> ---
>
> And the (concrete) class "ExceptionContext" would contain the implementation
> required for the "map" feature:
> ---
> public class ExceptionContext {
> void addMessage(Localizable pattern,
> Object ... arguments);
> void setValue(String key, Object value);
> Object getValue(String key);
> Set<String> getContextKeys();
> String getMessage(final Locale locale);
> String getMessage(final Locale locale,
> final String separator);
> }
> ---
>
> It would thus be less disruptive if a method is added later in the
> "ExceptionContext" utility.
>
> OK?
This is an elegant solution, I fully agree with it.
Luc
>
>
> Gilles
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] Restoring IAE to MathUtils#binomialCoefficient methods
Posted by Gilles Sadowski <gi...@harfang.homelinux.org>.
On Tue, May 03, 2011 at 01:08:29AM +0200, Gilles Sadowski wrote:
> > > [...]
> > >>Consistency implies that *all* exceptions thrown from CM must behave the
> > >>same way. I thus propose to add an interface like (maybe a better name?):
> > >>---
> > >>interface ContextedException {
> > >> void addMessage(Localizable pattern,
> > >> Object ... arguments);
> > >> void setContext(String key, Object value);
> > >> Object getContext(String key);
> > >> Set<String> getContextKeys();
> > >> String getMessage(final Locale locale);
> > >> String getMessage(final Locale locale,
> > >> final String separator);
> > >>}
> > >>And all CM exceptions will implement this interface. [Instead of
> > >>automatically inheriting the behaviour by being subclasses of
> > >>"MathRuntimeException".]
> > >>
> > >I would prefer as stated above to revert to actual RTEs per 2.x
> > >behavior. Above would be an improvement, as at least the unexpected
> > >RTEs at upgrade would not bite (as they did me), but I see no reason
> > >to add this machinery which is no less complex than what we had in
> > >2.x. Lets see what others think.
> >
> > Do the above mean we would have:
> >
> > public class MathIllegalArgumentException
> > extends IllegalArgumentException
> > implements ContextedException
>
> Yes. [Not sure about the name "ContextedException" yet.]
>
> > If so, then I am OK with this.
I've just been thinking that, as per the concerns raised in the thread about
interface overuse, it would be better to create a "thin" interface:
---
public interface ExceptionContextProvider {
/**
* @return the context data structure that enables the customization of
* the error message.
*/
ExceptionContext getContext();
}
---
Then, all CM exceptions would be like
---
public class MathIllegalArgumentException extends IllegalArgumentException
implements ExceptionContextProvider {
// ...
}
---
And the (concrete) class "ExceptionContext" would contain the implementation
required for the "map" feature:
---
public class ExceptionContext {
void addMessage(Localizable pattern,
Object ... arguments);
void setValue(String key, Object value);
Object getValue(String key);
Set<String> getContextKeys();
String getMessage(final Locale locale);
String getMessage(final Locale locale,
final String separator);
}
---
It would thus be less disruptive if a method is added later in the
"ExceptionContext" utility.
OK?
Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] Restoring IAE to MathUtils#binomialCoefficient methods
Posted by Gilles Sadowski <gi...@harfang.homelinux.org>.
> > [...]
> >>Consistency implies that *all* exceptions thrown from CM must behave the
> >>same way. I thus propose to add an interface like (maybe a better name?):
> >>---
> >>interface ContextedException {
> >> void addMessage(Localizable pattern,
> >> Object ... arguments);
> >> void setContext(String key, Object value);
> >> Object getContext(String key);
> >> Set<String> getContextKeys();
> >> String getMessage(final Locale locale);
> >> String getMessage(final Locale locale,
> >> final String separator);
> >>}
> >>And all CM exceptions will implement this interface. [Instead of
> >>automatically inheriting the behaviour by being subclasses of
> >>"MathRuntimeException".]
> >>
> >I would prefer as stated above to revert to actual RTEs per 2.x
> >behavior. Above would be an improvement, as at least the unexpected
> >RTEs at upgrade would not bite (as they did me), but I see no reason
> >to add this machinery which is no less complex than what we had in
> >2.x. Lets see what others think.
>
> Do the above mean we would have:
>
> public class MathIllegalArgumentException
> extends IllegalArgumentException
> implements ContextedException
Yes. [Not sure about the name "ContextedException" yet.]
> If so, then I am OK with this.
Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] Restoring IAE to MathUtils#binomialCoefficient methods
Posted by Luc Maisonobe <Lu...@free.fr>.
Le 02/05/2011 17:07, Phil Steitz a écrit :
> On 5/1/11 3:02 PM, Gilles Sadowski wrote:
>> On Sun, May 01, 2011 at 08:34:31AM -0700, Phil Steitz wrote:
>>> On 5/1/11 6:00 AM, Gilles Sadowski wrote:
>>>> On Sun, May 01, 2011 at 12:48:14PM +0200, Gilles Sadowski wrote:
>>>>> On Sat, Apr 30, 2011 at 10:53:30PM -0700, Phil Steitz wrote:
>>>>>> On 4/30/11 4:33 PM, Gilles Sadowski wrote:
>>>>>>> On Sat, Apr 30, 2011 at 09:10:08AM -0700, Phil Steitz wrote:
>>>>>>>> Converting some of my code to use trunk, I discovered that the
>>>>>>>> binomialCoefficient methods no longer throw IllegalArgumentException
>>>>>>>> when parameters are invalid.
>>>>>>> The consensus was a singly rooted hierarchy ("MathRuntimeException").
>>>>>>> The advantage being commonly agreed on was to offer the "map" functionality
>>>>>>> for adding messages and context information.
>>>>>> I guess I misunderstood and after really seeing the consequences in
>>>>>> my own code, I am going to have to ask that we reopen that
>>>>>> discussion - i.e., I would like us to throw IAE and other standard
>>>>>> exceptions where appropriate, as in this case, as we have up to and
>>>>>> through 2.2. I know I said before that I did not see this as worth
>>>>>> arguing about, but I really think this change is a bad API design
>>>>>> decision that will cause needless hassle and surprising RTEs for
>>>>>> users upgrading to the new version.
>>>>> I'm astonished, and for the time being, will refrain from other comments.
>>>>>
>>>> There is no one single best API design. What counts most IMO is that it is
>>>> consistent and leads to clean and lean code.
>>>> The old exception factories à la
>>>> -----
>>>> MathRuntimeException.createIllegalArgumentException("Error with an argument {0}", x);
>>>> -----
>>>> failed on all accounts.
>>>>
>>>> Now, I would like to settle this issue once for all. Should all CM
>>>> exceptions inherit from the standard Java exceptions hierarchies (rooted at
>>>> IAE, UOE, NPE, AE, etc.)? Although it had been answered "no" previously, it
>>>> was not my preferred choice. I could make it "yes" now but I'd rather not
>>>> changed that back and forth again and again.'
>>> I apologize sincerely for opening this up again. I don't think
>>> *all* exceptions thrown by [math] should derive from standard
>>> exceptions, other than I guess Exception itself. I do think,
>>> however, that [math] *should* throw standard exceptions directly
>>> when appropriate and in general favor standard exceptions. In
>>> particular, I would prefer that we revert to the 1.0-2.2 behavior of
>>> throwing IllegalArgumentException when preconditions are violated.
>>> I personally have no problem with using the exception factories and
>>> will volunteer to retrofit the code if we can agree to this change.
>> We agreed to provide enhanced "context-enabled" exceptions as a feature to
>> users. Throwing plain standard exceptions contradicts that goal.
>> I propose to rework the hierarchies so that MathIAE will inherit from the
>> standard IAE. This will solve the annoyance you would have had when
>> upgrading your code. [And we'll keep the "addMessage" feature together with
>> accessors like "getArgument" and "getMax" etc.]
>> The exception factories were a bad design idea (IMHO of course; plenty of
>> arguments given elsewhere in the last 6-8 months). One of the goals of the
>> exceptions refactoring was to get rid of them.
>>
>>> Once again, I hate to flip-flop, but this is an important and
>>> impactful decision and I have now experienced the upgrade pain
>>> (unexpected RTEs when upgrading) directly so would like to ask that
>>> we revisit it.
>> Not "unexpected RTEs". Incompatibilities are to be expected.
>> Anyways, this won't happen with the new-new solution.
>>
>>> To be clear, my opinion is that for all of the RTEs currently
>>> generated by MathRuntimeException.createXxxException, we should
>>> generate and throw these exceptions directly, as appropriate, using
>>> the factory to enable localization.
>>>
>> No, I like the current design; I didn't like the one you want to revert to.
>> Consistency implies that *all* exceptions thrown from CM must behave the
>> same way. I thus propose to add an interface like (maybe a better name?):
>> ---
>> interface ContextedException {
>> void addMessage(Localizable pattern,
>> Object ... arguments);
>> void setContext(String key, Object value);
>> Object getContext(String key);
>> Set<String> getContextKeys();
>> String getMessage(final Locale locale);
>> String getMessage(final Locale locale,
>> final String separator);
>> }
>> And all CM exceptions will implement this interface. [Instead of
>> automatically inheriting the behaviour by being subclasses of
>> "MathRuntimeException".]
>>
> I would prefer as stated above to revert to actual RTEs per 2.x
> behavior. Above would be an improvement, as at least the unexpected
> RTEs at upgrade would not bite (as they did me), but I see no reason
> to add this machinery which is no less complex than what we had in
> 2.x. Lets see what others think.
Do the above mean we would have:
public class MathIllegalArgumentException
extends IllegalArgumentException
implements ContextedException
If so, then I am OK with this.
Luc
>
> Phil
>> Gilles
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] Restoring IAE to MathUtils#binomialCoefficient methods
Posted by Phil Steitz <ph...@gmail.com>.
On 5/1/11 3:02 PM, Gilles Sadowski wrote:
> On Sun, May 01, 2011 at 08:34:31AM -0700, Phil Steitz wrote:
>> On 5/1/11 6:00 AM, Gilles Sadowski wrote:
>>> On Sun, May 01, 2011 at 12:48:14PM +0200, Gilles Sadowski wrote:
>>>> On Sat, Apr 30, 2011 at 10:53:30PM -0700, Phil Steitz wrote:
>>>>> On 4/30/11 4:33 PM, Gilles Sadowski wrote:
>>>>>> On Sat, Apr 30, 2011 at 09:10:08AM -0700, Phil Steitz wrote:
>>>>>>> Converting some of my code to use trunk, I discovered that the
>>>>>>> binomialCoefficient methods no longer throw IllegalArgumentException
>>>>>>> when parameters are invalid.
>>>>>> The consensus was a singly rooted hierarchy ("MathRuntimeException").
>>>>>> The advantage being commonly agreed on was to offer the "map" functionality
>>>>>> for adding messages and context information.
>>>>> I guess I misunderstood and after really seeing the consequences in
>>>>> my own code, I am going to have to ask that we reopen that
>>>>> discussion - i.e., I would like us to throw IAE and other standard
>>>>> exceptions where appropriate, as in this case, as we have up to and
>>>>> through 2.2. I know I said before that I did not see this as worth
>>>>> arguing about, but I really think this change is a bad API design
>>>>> decision that will cause needless hassle and surprising RTEs for
>>>>> users upgrading to the new version.
>>>> I'm astonished, and for the time being, will refrain from other comments.
>>>>
>>> There is no one single best API design. What counts most IMO is that it is
>>> consistent and leads to clean and lean code.
>>> The old exception factories à la
>>> -----
>>> MathRuntimeException.createIllegalArgumentException("Error with an argument {0}", x);
>>> -----
>>> failed on all accounts.
>>>
>>> Now, I would like to settle this issue once for all. Should all CM
>>> exceptions inherit from the standard Java exceptions hierarchies (rooted at
>>> IAE, UOE, NPE, AE, etc.)? Although it had been answered "no" previously, it
>>> was not my preferred choice. I could make it "yes" now but I'd rather not
>>> changed that back and forth again and again.'
>> I apologize sincerely for opening this up again. I don't think
>> *all* exceptions thrown by [math] should derive from standard
>> exceptions, other than I guess Exception itself. I do think,
>> however, that [math] *should* throw standard exceptions directly
>> when appropriate and in general favor standard exceptions. In
>> particular, I would prefer that we revert to the 1.0-2.2 behavior of
>> throwing IllegalArgumentException when preconditions are violated.
>> I personally have no problem with using the exception factories and
>> will volunteer to retrofit the code if we can agree to this change.
> We agreed to provide enhanced "context-enabled" exceptions as a feature to
> users. Throwing plain standard exceptions contradicts that goal.
> I propose to rework the hierarchies so that MathIAE will inherit from the
> standard IAE. This will solve the annoyance you would have had when
> upgrading your code. [And we'll keep the "addMessage" feature together with
> accessors like "getArgument" and "getMax" etc.]
> The exception factories were a bad design idea (IMHO of course; plenty of
> arguments given elsewhere in the last 6-8 months). One of the goals of the
> exceptions refactoring was to get rid of them.
>
>> Once again, I hate to flip-flop, but this is an important and
>> impactful decision and I have now experienced the upgrade pain
>> (unexpected RTEs when upgrading) directly so would like to ask that
>> we revisit it.
> Not "unexpected RTEs". Incompatibilities are to be expected.
> Anyways, this won't happen with the new-new solution.
>
>> To be clear, my opinion is that for all of the RTEs currently
>> generated by MathRuntimeException.createXxxException, we should
>> generate and throw these exceptions directly, as appropriate, using
>> the factory to enable localization.
>>
> No, I like the current design; I didn't like the one you want to revert to.
> Consistency implies that *all* exceptions thrown from CM must behave the
> same way. I thus propose to add an interface like (maybe a better name?):
> ---
> interface ContextedException {
> void addMessage(Localizable pattern,
> Object ... arguments);
> void setContext(String key, Object value);
> Object getContext(String key);
> Set<String> getContextKeys();
> String getMessage(final Locale locale);
> String getMessage(final Locale locale,
> final String separator);
> }
> And all CM exceptions will implement this interface. [Instead of
> automatically inheriting the behaviour by being subclasses of
> "MathRuntimeException".]
>
I would prefer as stated above to revert to actual RTEs per 2.x
behavior. Above would be an improvement, as at least the unexpected
RTEs at upgrade would not bite (as they did me), but I see no reason
to add this machinery which is no less complex than what we had in
2.x. Lets see what others think.
Phil
> Gilles
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] Restoring IAE to MathUtils#binomialCoefficient methods
Posted by Gilles Sadowski <gi...@harfang.homelinux.org>.
On Sun, May 01, 2011 at 08:34:31AM -0700, Phil Steitz wrote:
> On 5/1/11 6:00 AM, Gilles Sadowski wrote:
> > On Sun, May 01, 2011 at 12:48:14PM +0200, Gilles Sadowski wrote:
> >> On Sat, Apr 30, 2011 at 10:53:30PM -0700, Phil Steitz wrote:
> >>> On 4/30/11 4:33 PM, Gilles Sadowski wrote:
> >>>> On Sat, Apr 30, 2011 at 09:10:08AM -0700, Phil Steitz wrote:
> >>>>> Converting some of my code to use trunk, I discovered that the
> >>>>> binomialCoefficient methods no longer throw IllegalArgumentException
> >>>>> when parameters are invalid.
> >>>> The consensus was a singly rooted hierarchy ("MathRuntimeException").
> >>>> The advantage being commonly agreed on was to offer the "map" functionality
> >>>> for adding messages and context information.
> >>> I guess I misunderstood and after really seeing the consequences in
> >>> my own code, I am going to have to ask that we reopen that
> >>> discussion - i.e., I would like us to throw IAE and other standard
> >>> exceptions where appropriate, as in this case, as we have up to and
> >>> through 2.2. I know I said before that I did not see this as worth
> >>> arguing about, but I really think this change is a bad API design
> >>> decision that will cause needless hassle and surprising RTEs for
> >>> users upgrading to the new version.
> >> I'm astonished, and for the time being, will refrain from other comments.
> >>
> > There is no one single best API design. What counts most IMO is that it is
> > consistent and leads to clean and lean code.
> > The old exception factories à la
> > -----
> > MathRuntimeException.createIllegalArgumentException("Error with an argument {0}", x);
> > -----
> > failed on all accounts.
> >
> > Now, I would like to settle this issue once for all. Should all CM
> > exceptions inherit from the standard Java exceptions hierarchies (rooted at
> > IAE, UOE, NPE, AE, etc.)? Although it had been answered "no" previously, it
> > was not my preferred choice. I could make it "yes" now but I'd rather not
> > changed that back and forth again and again.'
> I apologize sincerely for opening this up again. I don't think
> *all* exceptions thrown by [math] should derive from standard
> exceptions, other than I guess Exception itself. I do think,
> however, that [math] *should* throw standard exceptions directly
> when appropriate and in general favor standard exceptions. In
> particular, I would prefer that we revert to the 1.0-2.2 behavior of
> throwing IllegalArgumentException when preconditions are violated.
> I personally have no problem with using the exception factories and
> will volunteer to retrofit the code if we can agree to this change.
We agreed to provide enhanced "context-enabled" exceptions as a feature to
users. Throwing plain standard exceptions contradicts that goal.
I propose to rework the hierarchies so that MathIAE will inherit from the
standard IAE. This will solve the annoyance you would have had when
upgrading your code. [And we'll keep the "addMessage" feature together with
accessors like "getArgument" and "getMax" etc.]
The exception factories were a bad design idea (IMHO of course; plenty of
arguments given elsewhere in the last 6-8 months). One of the goals of the
exceptions refactoring was to get rid of them.
> Once again, I hate to flip-flop, but this is an important and
> impactful decision and I have now experienced the upgrade pain
> (unexpected RTEs when upgrading) directly so would like to ask that
> we revisit it.
Not "unexpected RTEs". Incompatibilities are to be expected.
Anyways, this won't happen with the new-new solution.
> To be clear, my opinion is that for all of the RTEs currently
> generated by MathRuntimeException.createXxxException, we should
> generate and throw these exceptions directly, as appropriate, using
> the factory to enable localization.
>
No, I like the current design; I didn't like the one you want to revert to.
Consistency implies that *all* exceptions thrown from CM must behave the
same way. I thus propose to add an interface like (maybe a better name?):
---
interface ContextedException {
void addMessage(Localizable pattern,
Object ... arguments);
void setContext(String key, Object value);
Object getContext(String key);
Set<String> getContextKeys();
String getMessage(final Locale locale);
String getMessage(final Locale locale,
final String separator);
}
And all CM exceptions will implement this interface. [Instead of
automatically inheriting the behaviour by being subclasses of
"MathRuntimeException".]
Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] Restoring IAE to MathUtils#binomialCoefficient methods
Posted by Phil Steitz <ph...@gmail.com>.
On 5/1/11 6:00 AM, Gilles Sadowski wrote:
> On Sun, May 01, 2011 at 12:48:14PM +0200, Gilles Sadowski wrote:
>> On Sat, Apr 30, 2011 at 10:53:30PM -0700, Phil Steitz wrote:
>>> On 4/30/11 4:33 PM, Gilles Sadowski wrote:
>>>> On Sat, Apr 30, 2011 at 09:10:08AM -0700, Phil Steitz wrote:
>>>>> Converting some of my code to use trunk, I discovered that the
>>>>> binomialCoefficient methods no longer throw IllegalArgumentException
>>>>> when parameters are invalid.
>>>> The consensus was a singly rooted hierarchy ("MathRuntimeException").
>>>> The advantage being commonly agreed on was to offer the "map" functionality
>>>> for adding messages and context information.
>>> I guess I misunderstood and after really seeing the consequences in
>>> my own code, I am going to have to ask that we reopen that
>>> discussion - i.e., I would like us to throw IAE and other standard
>>> exceptions where appropriate, as in this case, as we have up to and
>>> through 2.2. I know I said before that I did not see this as worth
>>> arguing about, but I really think this change is a bad API design
>>> decision that will cause needless hassle and surprising RTEs for
>>> users upgrading to the new version.
>> I'm astonished, and for the time being, will refrain from other comments.
>>
> There is no one single best API design. What counts most IMO is that it is
> consistent and leads to clean and lean code.
> The old exception factories à la
> -----
> MathRuntimeException.createIllegalArgumentException("Error with an argument {0}", x);
> -----
> failed on all accounts.
>
> Now, I would like to settle this issue once for all. Should all CM
> exceptions inherit from the standard Java exceptions hierarchies (rooted at
> IAE, UOE, NPE, AE, etc.)? Although it had been answered "no" previously, it
> was not my preferred choice. I could make it "yes" now but I'd rather not
> changed that back and forth again and again.'
I apologize sincerely for opening this up again. I don't think
*all* exceptions thrown by [math] should derive from standard
exceptions, other than I guess Exception itself. I do think,
however, that [math] *should* throw standard exceptions directly
when appropriate and in general favor standard exceptions. In
particular, I would prefer that we revert to the 1.0-2.2 behavior of
throwing IllegalArgumentException when preconditions are violated.
I personally have no problem with using the exception factories and
will volunteer to retrofit the code if we can agree to this change.
Once again, I hate to flip-flop, but this is an important and
impactful decision and I have now experienced the upgrade pain
(unexpected RTEs when upgrading) directly so would like to ask that
we revisit it.
To be clear, my opinion is that for all of the RTEs currently
generated by MathRuntimeException.createXxxException, we should
generate and throw these exceptions directly, as appropriate, using
the factory to enable localization.
Phil
>
> Gilles
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] Restoring IAE to MathUtils#binomialCoefficient methods
Posted by Luc Maisonobe <Lu...@free.fr>.
Le 01/05/2011 15:00, Gilles Sadowski a écrit :
> On Sun, May 01, 2011 at 12:48:14PM +0200, Gilles Sadowski wrote:
>> On Sat, Apr 30, 2011 at 10:53:30PM -0700, Phil Steitz wrote:
>>> On 4/30/11 4:33 PM, Gilles Sadowski wrote:
>>>> On Sat, Apr 30, 2011 at 09:10:08AM -0700, Phil Steitz wrote:
>>>>> Converting some of my code to use trunk, I discovered that the
>>>>> binomialCoefficient methods no longer throw IllegalArgumentException
>>>>> when parameters are invalid.
>>>> The consensus was a singly rooted hierarchy ("MathRuntimeException").
>>>> The advantage being commonly agreed on was to offer the "map" functionality
>>>> for adding messages and context information.
>>> I guess I misunderstood and after really seeing the consequences in
>>> my own code, I am going to have to ask that we reopen that
>>> discussion - i.e., I would like us to throw IAE and other standard
>>> exceptions where appropriate, as in this case, as we have up to and
>>> through 2.2. I know I said before that I did not see this as worth
>>> arguing about, but I really think this change is a bad API design
>>> decision that will cause needless hassle and surprising RTEs for
>>> users upgrading to the new version.
>>
>> I'm astonished, and for the time being, will refrain from other comments.
>>
>
> There is no one single best API design. What counts most IMO is that it is
> consistent and leads to clean and lean code.
> The old exception factories à la
> -----
> MathRuntimeException.createIllegalArgumentException("Error with an argument {0}", x);
> -----
> failed on all accounts.
>
> Now, I would like to settle this issue once for all.
Agreed.
> Should all CM
> exceptions inherit from the standard Java exceptions hierarchies (rooted at
> IAE, UOE, NPE, AE, etc.)? Although it had been answered "no" previously, it
> was not my preferred choice.
Choose as you want. I won't voice any opinion here.
> I could make it "yes" now but I'd rather not
> changed that back and forth again and again.
Agreed.
Luc
>
>
> Gilles
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] Restoring IAE to MathUtils#binomialCoefficient methods
Posted by Gilles Sadowski <gi...@harfang.homelinux.org>.
On Sun, May 01, 2011 at 12:48:14PM +0200, Gilles Sadowski wrote:
> On Sat, Apr 30, 2011 at 10:53:30PM -0700, Phil Steitz wrote:
> > On 4/30/11 4:33 PM, Gilles Sadowski wrote:
> > > On Sat, Apr 30, 2011 at 09:10:08AM -0700, Phil Steitz wrote:
> > >> Converting some of my code to use trunk, I discovered that the
> > >> binomialCoefficient methods no longer throw IllegalArgumentException
> > >> when parameters are invalid.
> > > The consensus was a singly rooted hierarchy ("MathRuntimeException").
> > > The advantage being commonly agreed on was to offer the "map" functionality
> > > for adding messages and context information.
> > I guess I misunderstood and after really seeing the consequences in
> > my own code, I am going to have to ask that we reopen that
> > discussion - i.e., I would like us to throw IAE and other standard
> > exceptions where appropriate, as in this case, as we have up to and
> > through 2.2. I know I said before that I did not see this as worth
> > arguing about, but I really think this change is a bad API design
> > decision that will cause needless hassle and surprising RTEs for
> > users upgrading to the new version.
>
> I'm astonished, and for the time being, will refrain from other comments.
>
There is no one single best API design. What counts most IMO is that it is
consistent and leads to clean and lean code.
The old exception factories à la
-----
MathRuntimeException.createIllegalArgumentException("Error with an argument {0}", x);
-----
failed on all accounts.
Now, I would like to settle this issue once for all. Should all CM
exceptions inherit from the standard Java exceptions hierarchies (rooted at
IAE, UOE, NPE, AE, etc.)? Although it had been answered "no" previously, it
was not my preferred choice. I could make it "yes" now but I'd rather not
changed that back and forth again and again.
Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] Restoring IAE to MathUtils#binomialCoefficient methods
Posted by Gilles Sadowski <gi...@harfang.homelinux.org>.
On Sat, Apr 30, 2011 at 10:53:30PM -0700, Phil Steitz wrote:
> On 4/30/11 4:33 PM, Gilles Sadowski wrote:
> > On Sat, Apr 30, 2011 at 09:10:08AM -0700, Phil Steitz wrote:
> >> Converting some of my code to use trunk, I discovered that the
> >> binomialCoefficient methods no longer throw IllegalArgumentException
> >> when parameters are invalid.
> > The consensus was a singly rooted hierarchy ("MathRuntimeException").
> > The advantage being commonly agreed on was to offer the "map" functionality
> > for adding messages and context information.
> I guess I misunderstood and after really seeing the consequences in
> my own code, I am going to have to ask that we reopen that
> discussion - i.e., I would like us to throw IAE and other standard
> exceptions where appropriate, as in this case, as we have up to and
> through 2.2. I know I said before that I did not see this as worth
> arguing about, but I really think this change is a bad API design
> decision that will cause needless hassle and surprising RTEs for
> users upgrading to the new version.
I'm astonished, and for the time being, will refrain from other comments.
> >> The javadoc asserts that
> >> MathIllegalArgumentException will be thrown in these cases, but that
> >> is not correct,
> > I don't understand; the code for "checkBinomial" can throw
> > "NumberIsTooLargeException" or "NotPositiveException" and they are
> > subclasses of "MathIllegalArgumentException".
> >
> Sorry. my mistake.
> >> since what is actually thrown now can differ
> >> depending on the parameter problem
> > That's a feature, naturally: Different problem, different exception.
> >
> That's where I disagree. I see zero value added and in fact
> confusing complexity introduced by these exceptions. When you ask
> for B(n,k) where k is not less than or equal to n, a standard IAE
> with a message that says precisely that (which the current message
> does) is clear and *better* that a "NumberIsTooLargeException".
> What number? I guess it must be k? To figure it out you have to
> look at the exception message, which is *exactly the same message*
> that the old code reported. If we really think we need to
> specialize and report different exceptions for every precondition
> violation (which I do not agree with), then these exceptions should
> be meaningful in the context of the API that is using them. So
> here, for example, we would have to throw something like
> "CombinationSizeTooLargeForSetException."
Then, we do _not_ disagree _now_. From the start, I stated that a consistent
design would be to define a specific exception for each specific that must
be reported, especially if it must contain complex functionality like
localization.
IIRC, either you or Luc (or both) did not want a large number of exceptions.
To keep the number down, we reuse less specific exception types (like
"NumberIsTooLargeException" in several contexts) and rely on the message(s)
for context information. Nothing lost from the previous situation (when one
*had* to rely solely on the message)!
To answer your question above: No, you don't have to "guess" which number is
too large; there is an accessor "getArgument()" that returns the number that
triggered the exception and another "getMax()" that informs you about the
maximum allowed number.
> >> and the resulting exceptions are
> >> neither standard IAEs nor descendents of MathIAE.
> > >From what I see, they are descendents of MathIAE.
> >
> >> I have patched
> >> the code to return a standard IAE (with localized message). Per
> >> discussion in [1] it looks like we were close to consensus to favor
> >> standard exceptions and in this case,
> > No, that thread was discussing
> > throwing standard "NullPointerException"
> > vs
> > throwing a CM-specific "NullArgumentException" (subtype of MathIAE)
> > vs
> > not checking for null pointer at all.
> > [I don't think that a consensus has been reached on that issue.]
> >
> > For all the other cases of invalid parameters passed to methods, it had
> > been settled already that "MathIllegalArgumentException" (or subclasses
> > thereof) would been thrown.
> >
> >> I would much rather return a
> >> standard IAE with meaningful error message rather than a
> >> non-standard RTE (with exactly the same error message and generally
> >> confusing type - e.g. "NumberIsTooSmall" when n, k parameters are
> >> not in the right order) and keep the javadoc simple. Otherwise, the
> >> main method javadoc has to be rewritten to conform to what the code
> >> now does.
> > The Javadoc "@throws" tag is not incorrect:
> > -----
> > * @throws MathIllegalArgumentException if preconditions are not met
> > -----
> > But it is not as precise as it could (by mentioning the types actually
> > thrown by "checkBinomial").
> > The main description is indeed a remnant of the old behaviour and it is yet
> > another proof that it is not good documentation practise to repeat the
> > (supposedly) same information several times.
> > Documentation for methods "binomialCoefficientDouble" and
> > "binomialCoefficientLog" also refer to the old behaviour and must be
> > updated.
> Regardless of how we settle this, we *must* keep the javadoc
> consistent with what the code does and we *must* document fully both
> preconditions and exceptions thrown.
Certainly, please open a JIRA ticket for this specific issue.
Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] Restoring IAE to MathUtils#binomialCoefficient methods
Posted by Phil Steitz <ph...@gmail.com>.
On 4/30/11 4:33 PM, Gilles Sadowski wrote:
> On Sat, Apr 30, 2011 at 09:10:08AM -0700, Phil Steitz wrote:
>> Converting some of my code to use trunk, I discovered that the
>> binomialCoefficient methods no longer throw IllegalArgumentException
>> when parameters are invalid.
> The consensus was a singly rooted hierarchy ("MathRuntimeException").
> The advantage being commonly agreed on was to offer the "map" functionality
> for adding messages and context information.
I guess I misunderstood and after really seeing the consequences in
my own code, I am going to have to ask that we reopen that
discussion - i.e., I would like us to throw IAE and other standard
exceptions where appropriate, as in this case, as we have up to and
through 2.2. I know I said before that I did not see this as worth
arguing about, but I really think this change is a bad API design
decision that will cause needless hassle and surprising RTEs for
users upgrading to the new version.
>> The javadoc asserts that
>> MathIllegalArgumentException will be thrown in these cases, but that
>> is not correct,
> I don't understand; the code for "checkBinomial" can throw
> "NumberIsTooLargeException" or "NotPositiveException" and they are
> subclasses of "MathIllegalArgumentException".
>
Sorry. my mistake.
>> since what is actually thrown now can differ
>> depending on the parameter problem
> That's a feature, naturally: Different problem, different exception.
>
That's where I disagree. I see zero value added and in fact
confusing complexity introduced by these exceptions. When you ask
for B(n,k) where k is not less than or equal to n, a standard IAE
with a message that says precisely that (which the current message
does) is clear and *better* that a "NumberIsTooLargeException".
What number? I guess it must be k? To figure it out you have to
look at the exception message, which is *exactly the same message*
that the old code reported. If we really think we need to
specialize and report different exceptions for every precondition
violation (which I do not agree with), then these exceptions should
be meaningful in the context of the API that is using them. So
here, for example, we would have to throw something like
"CombinationSizeTooLargeForSetException."
>> and the resulting exceptions are
>> neither standard IAEs nor descendents of MathIAE.
> >From what I see, they are descendents of MathIAE.
>
>> I have patched
>> the code to return a standard IAE (with localized message). Per
>> discussion in [1] it looks like we were close to consensus to favor
>> standard exceptions and in this case,
> No, that thread was discussing
> throwing standard "NullPointerException"
> vs
> throwing a CM-specific "NullArgumentException" (subtype of MathIAE)
> vs
> not checking for null pointer at all.
> [I don't think that a consensus has been reached on that issue.]
>
> For all the other cases of invalid parameters passed to methods, it had
> been settled already that "MathIllegalArgumentException" (or subclasses
> thereof) would been thrown.
>
>> I would much rather return a
>> standard IAE with meaningful error message rather than a
>> non-standard RTE (with exactly the same error message and generally
>> confusing type - e.g. "NumberIsTooSmall" when n, k parameters are
>> not in the right order) and keep the javadoc simple. Otherwise, the
>> main method javadoc has to be rewritten to conform to what the code
>> now does.
> The Javadoc "@throws" tag is not incorrect:
> -----
> * @throws MathIllegalArgumentException if preconditions are not met
> -----
> But it is not as precise as it could (by mentioning the types actually
> thrown by "checkBinomial").
> The main description is indeed a remnant of the old behaviour and it is yet
> another proof that it is not good documentation practise to repeat the
> (supposedly) same information several times.
> Documentation for methods "binomialCoefficientDouble" and
> "binomialCoefficientLog" also refer to the old behaviour and must be
> updated.
Regardless of how we settle this, we *must* keep the javadoc
consistent with what the code does and we *must* document fully both
preconditions and exceptions thrown.
Phil
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org