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-unsubscr...@commons.apache.org
> > For additional commands, e-mail: dev-h...@commons.apache.org
> >
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> For additional commands, e-mail: dev-h...@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
