commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gilles Sadowski <gil...@harfang.homelinux.org>
Subject Re: svn commit: r1134939 - /commons/proper/math/trunk/src/main/java/org/apache/commons/math/stat/inference/ChiSquareTestImpl.java
Date Mon, 13 Jun 2011 10:38:20 GMT
On Sun, Jun 12, 2011 at 02:19:11PM -0700, Phil Steitz wrote:
> On 6/12/11 1:41 PM, Gilles Sadowski wrote:
> > On Sun, Jun 12, 2011 at 03:57:32PM -0000, psteitz@apache.org wrote:
> >> Author: psteitz
> >> Date: Sun Jun 12 15:57:32 2011
> >> New Revision: 1134939
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1134939&view=rev
> >> Log:
> >> Restored specificity in exception error messages.
> >>
> >> Modified:
> >>     commons/proper/math/trunk/src/main/java/org/apache/commons/math/stat/inference/ChiSquareTestImpl.java
> >>
> >> Modified: commons/proper/math/trunk/src/main/java/org/apache/commons/math/stat/inference/ChiSquareTestImpl.java
> >> URL: http://svn.apache.org/viewvc/commons/proper/math/trunk/src/main/java/org/apache/commons/math/stat/inference/ChiSquareTestImpl.java?rev=1134939&r1=1134938&r2=1134939&view=diff
> >> ==============================================================================
> >> --- commons/proper/math/trunk/src/main/java/org/apache/commons/math/stat/inference/ChiSquareTestImpl.java
(original)
> >> +++ commons/proper/math/trunk/src/main/java/org/apache/commons/math/stat/inference/ChiSquareTestImpl.java
Sun Jun 12 15:57:32 2011
> >> @@ -20,7 +20,6 @@ import org.apache.commons.math.MathExcep
> >>  import org.apache.commons.math.exception.NotPositiveException;
> >>  import org.apache.commons.math.exception.NotStrictlyPositiveException;
> >>  import org.apache.commons.math.exception.NullArgumentException;
> >> -import org.apache.commons.math.exception.NumberIsTooSmallException;
> >>  import org.apache.commons.math.exception.OutOfRangeException;
> >>  import org.apache.commons.math.exception.DimensionMismatchException;
> >>  import org.apache.commons.math.exception.MathIllegalArgumentException;
> >> @@ -321,11 +320,13 @@ public class ChiSquareTestImpl implement
> >>       */
> >>      private void checkArray(long[][] in) {
> >>          if (in.length < 2) {
> >> -            throw new NumberIsTooSmallException(in.length, 2, true);
> >> +            throw new MathIllegalArgumentException(
> >> +                    LocalizedFormats.INSUFFICIENT_DIMENSION, in.length, 2);
> >>          }
> > It's preferable to keep a specific exception type when one exists.
> > If a contextual message is necessary, it can be added through the
> > "ExceptionContext":
> >
> > E.g. for the above code excerpt:
> > ---CUT---
> >     private void checkArray(long[][] in) {
> > 	final int len = in.length;
> >         final int minLen = 2;
> >         if (len < minLen) {
> >             MathIllegalArgumentException err = new NumberIsTooSmallException(len,
minLen, true);
> >             err.getContext().addMessage(LocalizedFormats.INSUFFICIENT_DIMENSION,
len, minLen);
> >             throw err;
> >         }
> >     }
> > ---CUT---
> >
> > And similarly for all the changes in this commit.
> 
> In this case, we either need to define new specific exceptions

+1

> or
> (my preference) leave as is.  Why add all of the code above to add
> no additional meaning to the IllegalArgumentException?  The problem
> in the first case is that (as stated in the exception message) the
> input data array has insufficient dimensionality.  Calling it a
> "NumberIsTooSmallException" adds nothing.

It's the other way around: the initial "cause" is that a number is too
small; that's exactly what is checked by the test
  if (in.length < 2)
The context is an additional information which can either be conveyed by
a subclass (my preference) or by using the "ExceptionContext".
The former will make it for even less code since the enum will be hidden in
the exception class (which is why I prefer it).
The ultimate goal is to avoid the numerous duplicates (enum elements with
about the same meaning) in "LocalizedFormats"; this can be achieved by only
using them in exception classes.

>  In the second case, the
> expected counts array needs to be fully non-negative, and the
> exception needs to report which element violates the condition. 
> There are several other IllegalArgumentExceptions used elsewhere in
> this class.  I see no reason to change them all to
> precondition-specific exceptions. 

If the same error appears in more than one class, it could be deemed a
good candidate for defining a new exception class.

Gilles

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message