On Fri, 30 Oct 2015 09:20:00 +0000, Eric Barnhill wrote:
On 30/10/15 02:15, Gilles wrote:
There are some problems with the Javadoc (wrong "@return" comment).
Not all local variables that are constant are declared "final".
I am happy to give it all another proof read. I take it the procedure
is to fork the dedicated branch and then submit a pull request to
that
branch?
I couldn't say; we don't have a procedure yet... :-}
Shouldn't independent changes be performed in separate commits?
[Referring to "IntegerSequence" and "LaguerreSolver".]
I made edits to the Solver in particular because my commit would
otherwise have left it broken.
Since you added new methods, that should not be the case.
Whenever possible, it's clearer to modify calls in other parts of the
library in a separate commit, and then remove obsolete methods in a
third step.
If that is not best practice I am happy
to revert and submit independently.
Actually, I don't like the new "size" method in "IntegerSequence".
Although the number of elements is needed in the new methods in
ComplexUtils, I don't think that we should advertise the
functionality
in its current (necessarily) poor implementation.
A better alternative is to have an instance that will hold the
number of elements it contains:
https://issues.apache.org/jira/browse/MATH-1286
My first impulse was to see if I could add a field to the range
object that contained its size. But as the method returns an Iterator
I didn't see a way to do that without returning a subclass that
extended Iterator in some way instead. I figured that was not a
desired outcome. So instead I incorporated what as far as I know is
best practice:
http://stackoverflow.com/questions/9720195/what-is-the-best-way-to-get-the-count-length-size-of-an-iterator
They don't really say it's best practice, rather, it's all you can
do with an Iterator. ;-)
Alternatively I could just create a local private method in
ComplexUtils that determined the size by iterating through the range
and call that, and leave IntegerSequence alone.
When MATH-1286 is resolved, that won't be necessary.
Best regards,
Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
