commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Rostislav Krasny (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (MATH-1307) Create a base class for all RNGs
Date Mon, 28 Dec 2015 18:48:49 GMT

    [ https://issues.apache.org/jira/browse/MATH-1307?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15072998#comment-15072998
] 

Rostislav Krasny commented on MATH-1307:
----------------------------------------

Taken from the attached java file:
{code:java}    public void nextBytes(byte[] bytes,
                          int start,
                          int len) {
        if (start < 0 ||
            start >= bytes.length) {
            throw new OutOfRangeException(start, 0, bytes.length);
        }
        final int max = bytes.length - start;
        if (len <= 0 ||
            len > max) {
            throw new OutOfRangeException(len, 0, max);
        }

        nextBytesFill(bytes, start, len);
    }{code}
I've a few comments about this code:
1. The {{len <= 0}} expression in the second {{if}} is wrong. Zero length must be considered
correct as well. I think it is a bad idea to deside instead of the CM user what's wrong in
his/her code. This should be the CM user's responsibility. But please don't add another {{if}}
for this case that just returns. Just change the expression to {{len < 0}}. It's better
to reach nextBytesFill() rarely with zero length than always check this case and add a work
to the branch predictor of the CPU https://en.wikipedia.org/wiki/Branch_predictor
2. Why {{bytes.length - start}} calculation isn't used in the {{if}} expression right away
but is saved in the {{max}} variable? The {{max}} variable is allocated on the stack, it used
only once, the calculation is trivial and this is just waste of resources. I think the {{len
> max}} is better to be changed to {{len > bytes.length - start}}
3. If you changed the {{nextBytes()}} method parameter names why not to do so in the {{nextBytesFill()}}
as well? And did you do this according to some naming convention? I think "offset" is more
common name than "start". I used the "position" name in the originally proposed code by analogy
with {{System.arraycopy()}} parameters.

> Create a base class for all RNGs
> --------------------------------
>
>                 Key: MATH-1307
>                 URL: https://issues.apache.org/jira/browse/MATH-1307
>             Project: Commons Math
>          Issue Type: Improvement
>            Reporter: Gilles
>            Assignee: Gilles
>            Priority: Minor
>              Labels: api, inheritance
>             Fix For: 4.0
>
>         Attachments: BaseRandomGenerator.java
>
>
> I proposed to create a base class which the existing abstract classes {{AbstractRandomGenerator}}
and {{BitsStreamGenerator}} will extend.
> This would allow to define {{nextBytes(byte[])}} at the base class level.
> The code for that method is almost identical in the two hierarchies: they only differ
in a call to either {{nextInt()}} or {{next(32)}} respectively; the latter is however the
same as the former, in disguise, and is not subject to change given the type of return value.
> As a corollary, the new base class can be the unique place where to add utilities such
as the one proposed in MATH-1306.
> *Update:* {{AbstractRandomGenerator}} and {{BitsStreamGenerator}} are both obsoleted
by the class proposed in this report.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message