commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Gilles (Updated) (JIRA)" <j...@apache.org>
Subject [jira] [Updated] (MATH-734) Code cleanup: "ISAACRandom"
Date Wed, 18 Jan 2012 15:36:41 GMT

     [ 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

        

Mime
View raw message