[
https://issues.apache.org/jira/browse/MATH-734?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Gilles updated MATH-734:
------------------------
Description:
In revision 1232899, I started to clean up the code (mainly, removing the
one-letter instance variables, that can easily be confused with local ones
within methods, making the code harder to understand and maintain).
Other points I'd want to handle:
* Should "Serializable" be implemented for such classes? I think not;
especially if it supposed to be used for "secure" applications.
* (Related to the above) I'd remove the "transient" keyword.
* The contents of method "allocArrays" should be moved to within the
constructor.
* It is not recommended to call non-final "public" methods ("setSeed") from
within the constructor, because an overriding code could access a not fully
uninitialized object.
* All initialization should take place within a single, most general,
constructor and the other constructors should call that one (using the
{{this(...)}} statement).
* This code (line 130)
{code}
if (seed == null) {
setSeed(System.currentTimeMillis() + System.identityHashCode(this));
return;
}
{code}
should probably be removed: it is safer to consider passing a null reference as
a user error.
* I'd remove the constructor taking a {{long}} argument. It is not obvious how
the code will use the argument. I'd rather offer that alternative, in the class
documentation, as an example of how to initialize the "array of integers"
argument.
was:
In revision 1232899, I started to clean up the code (mainly, removing the
one-letter instance variables, that can easily be confused with local ones
within methods, making the code harder to understand and maintain).
Other points I'd want to handle:
* Should "Serializable" be implemented for such classes? I think not;
especially if it supposed to be used for "secure" applications.
* (Related to the above) I'd remove the "transient" keyword.
* The contents of method "allocArrays" should be moved to within the
constructor.
* It is not recommended to call non-final "public" methods ("setSeed") from
within the constructor, because an overriding code could access a not fully
uninitialized object.
* All initialization should take place within a single, most general,
constructor and the other constructors should call that one (using the
{{this(...)}} statement).
* This code (line 130)
{code}
if (seed == null) {
setSeed(System.currentTimeMillis() + System.identityHashCode(this));
return;
}
{code}
should probably be removed: it is safer to consider passing a null reference as
a user error.
> Code cleanup: "ISAACRandom"
> ---------------------------
>
> Key: MATH-734
> URL: https://issues.apache.org/jira/browse/MATH-734
> Project: Commons Math
> Issue Type: Improvement
> Reporter: Gilles
> Assignee: Gilles
> Priority: Minor
> Fix For: 3.0
>
>
> In revision 1232899, I started to clean up the code (mainly, removing the
> one-letter instance variables, that can easily be confused with local ones
> within methods, making the code harder to understand and maintain).
> Other points I'd want to handle:
> * Should "Serializable" be implemented for such classes? I think not;
> especially if it supposed to be used for "secure" applications.
> * (Related to the above) I'd remove the "transient" keyword.
> * The contents of method "allocArrays" should be moved to within the
> constructor.
> * It is not recommended to call non-final "public" methods ("setSeed") from
> within the constructor, because an overriding code could access a not fully
> uninitialized object.
> * All initialization should take place within a single, most general,
> constructor and the other constructors should call that one (using the
> {{this(...)}} statement).
> * This code (line 130)
> {code}
> if (seed == null) {
> setSeed(System.currentTimeMillis() + System.identityHashCode(this));
> return;
> }
> {code}
> should probably be removed: it is safer to consider passing a null reference
> as a user error.
> * I'd remove the constructor taking a {{long}} argument. It is not obvious
> how the code will use the argument. I'd rather offer that alternative, in the
> class documentation, as an example of how to initialize the "array of
> integers" argument.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
