Hi Sébastien. > >> 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: [email protected] > > For additional commands, e-mail: [email protected] > > > > 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 question to ask is: What does it mean for "cb" to be null? IMO, it means nothing, and I'd tell that to the user by throwing an exception. Why would someone call the two-args constructor, whereas the one-arg constructor performs the exact same thing without ambiguity? > The first > constructor was indeed fine, but the new implementation of the second > constructor (including the test for null) would lead to duplicate > code. There is duplicate code only if you allow two ways to do the same thing. I.e. new Incrementor(max); or new Incrementor(max, null); It's unnecessary and confusing (because it's not obvious that the first call should behave like the second). > 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. That was already the case before. [You've just added a way to use the second constructor instead of the first one for doing the exact same thing.] > I should have thought that > avoiding code duplication in constructors was good practice. Yes it is of course. > > From your reaction, I assume you do not like the idea of null being a > meaningful value for the parameter cb? Exactly: If a user does not care about the callback, he shouldn't even have to look at the second constructor, even less wonder about the consequence of setting it to null. > 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. No ticket is necessary (IMHO). I'm fine with just reverting, then adding the precondition block: it will clearly express that a callback *is* necessary. [Thanks for spotting that bug.] Best, Gilles --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
