Hi Gilles,
2012/6/23 Gilles Sadowski <gil...@harfang.homelinux.org>: > Hello. > > On Sat, Jun 23, 2012 at 03:09:15PM -0000, celes...@apache.org wrote: >> Author: celestin >> Date: Sat Jun 23 15:09:14 2012 >> New Revision: 1353140 > > Either I don't understand this change, or I don't agree with it. > >> >> URL: http://svn.apache.org/viewvc?rev=1353140&view=rev >> Log: >> In o.a.c.m3.Incrementor, modified constructor to allow for null values of >> the MaxCountExceededCallback. Null value was previously not checked wich >> could lead to a NullPointerException much later (at exhaustion of the >> counter). >> >> Modified: >> >> commons/proper/math/trunk/src/main/java/org/apache/commons/math3/util/Incrementor.java >> >> Modified: >> commons/proper/math/trunk/src/main/java/org/apache/commons/math3/util/Incrementor.java >> URL: >> http://svn.apache.org/viewvc/commons/proper/math/trunk/src/main/java/org/apache/commons/math3/util/Incrementor.java?rev=1353140&r1=1353139&r2=1353140&view=diff >> ============================================================================== >> --- >> commons/proper/math/trunk/src/main/java/org/apache/commons/math3/util/Incrementor.java >> (original) >> +++ >> commons/proper/math/trunk/src/main/java/org/apache/commons/math3/util/Incrementor.java >> Sat Jun 23 15:09:14 2012 >> @@ -58,13 +58,7 @@ public class Incrementor { >> * @param max Maximal count. >> */ >> public Incrementor(int max) { >> - this(max, >> - new MaxCountExceededCallback() { >> - /** {@inheritDoc} */ >> - public void trigger(int max) { >> - throw new MaxCountExceededException(max); >> - } >> - }); >> + this(max, null); >> } > > You set the callback to "null" here but... > >> /** >> @@ -72,12 +66,22 @@ public class Incrementor { >> * counter exhaustion. >> * >> * @param max Maximal count. >> - * @param cb Function to be called when the maximal count has been >> reached. >> + * @param cb Function to be called when the maximal count has been >> reached >> + * (can be {@code null}). >> */ >> public Incrementor(int max, >> MaxCountExceededCallback cb) { >> maximalCount = max; >> - maxCountCallback = cb; >> + if (cb != null) { >> + maxCountCallback = cb; >> + } else { >> + maxCountCallback = new MaxCountExceededCallback() { >> + /** {@inheritDoc} */ >> + public void trigger(int max) { >> + throw new MaxCountExceededException(max); >> + } >> + }; >> + } >> } > > ... here, when it *is* null, you set it to something not null. So it seems > to be a contorted way of disallowing null while saying the opposite in the > Javadoc! > > The first constructor was fine as it was. > In the second, a check for not null was missing: > --- > public Incrementor(int max, > MaxCountExceededCallback cb) { > if (cb == null) { > throw new NullArgumentException(); > } > maximalCount = max; > maxCountCallback = cb; > } > --- > > Best regards, > Gilles > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > Well, the idea is this: if cb is null, fall back to default instead of throwing an exception, which I find quite unnecessary here. The first constructor was indeed fine, but the new implementation of the second constructor (including the test for null) would lead to duplicate code. So the first constructor, with restricted number of parameters (assuming default values for unspecified parameters) calls the second constructor, which is the most general one. I should have thought that avoiding code duplication in constructors was good practice. >From your reaction, I assume you do not like the idea of null being a meaningful value for the parameter cb? If so, I'm sorry. I really thought that this change was rather minor. If you like, I can cancel this change, and open a JIRA ticket. Best regards, Sébastien --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
