Hi. I had a lot of fun reading the source for this module. Here is some constructive criticism, ranging from the major to the pathologically pedantic.

  1. INIT blocks are broken in Perl 5. Use BEGIN blocks instead. (For more on this, see ihb's and ikegami's replies to me below this node. BTW, just in case it isn't clear already, in that exchange I am on the wrong side of the argument.)
  2. I understand the desire to make a drop-in replacement for Perl's rand (and srand), but I think it is a huge missed opportunity not to make this PRNG available as objects. PRNGs are a special kind of iterator; most, if not all, of the benefits of iterator objects apply to PRNG objects. Specifically, the possibility of having multiple independent PRNGs operating in parallel enables much greater modularization than having a single global PRNG like rand. This can be a real time saver, specially during testing and debugging.
  3. I agree that statistical tests are essential for the this module's test suite.
  4. I also agree with the opinion that _seed should be broken up. I would use a dispatch table, like this: 
    {
  # the values of the following hash are subs defined elsewhere
  my %dispatch = (
    dev_random  => \&_dev_random,
    dev_urandom => \&_dev_urandom,
    random_org  => \&_random_org,
    hotbits     => \&_hotbits,
  );

  sub _seed {
    my $none = $_[ 0 ] eq 'none';
    my @methods =  @_ if not $none;

    @seed = ();
    my $magic_num = magic_num();

    while ( @methods ) {
      my $method = shift @methods;

      my $need = $magic_num - @seed;

      if ( @methods &&
           looks_like_number( $methods[0] ) ) {
        my $n = shift @methods;
        $need = $n if $n < $need;
      }

      unless ( defined $dispatch{ $method } ) {
        push @errors, "Unsupported seeding method: $method";
        next;
      }
      push @seed, $dispatch{ $method }->( $need );
      last if @seed >= $magic_num;
    }

    push @errors, 'Full seed not acquired from sources: ' . @seed
      if @seed < $magic_num and not $none;

    push @seed, time, $$;
    $#seed = $magic_num - 1 if @seed > $magic_num;

    # Seed the PRNG
    seed( @seed );
  }
}

    [download]
  5. PRNG code often uses one or several "magic numbers", like the 624 that appears frequently in the implementation of _seed. As shown in the code given in the previous point, it is both easier to read and to maintain if one puts these into file-scoped variables or constants or accessors. (My preference is to use lexicals with corresponding getters or getter/setters, depending on the desired API.)
  6. I am confused by this bit in import 
            elsif ($sym =~ 'auto') {
            # To auto-seed or not
            $auto_seed = $sym =~ /no|!/;

        }

    [download]
    Don't you want $auto_seed to be set to $sym !~ /no|!/ ? If so, I would refactor that to something more like 
            elsif ($sym =~ /^(no|!)?auto$) {
          $auto_seed = not defined $1;
        }

    [download]
    ...although I must admit that I don't really understand the difference between importing 'noauto' and simply leaving 'auto' out of the import list altogether. If there is no difference, then I'd get rid of the 'noauto' option and change the fragment above to 
            elsif ($sym eq 'auto') {
          # To auto-seed or not
          $auto_seed = 1;  # default value for $auto_seed should be 0
        }

    [download]
  7. It might be nice to have access (e.g. through a getter) to the user agent object used when contacting remote sources over HTTP. This would be similar to what LWP::Simple provides, but using an accessor, instead of exporting the package variable.
  8. IMO the documentation could do a better job in explaining when someone may want to use this module over Perl's standard rand, or vice-versa.
  9. I think one should relax strict for the smallest scope that requires it. Hence, in import I'd move the no strict 'refs' to 
            {
           no strict 'refs';
           *{"${pkg}::$sym"} = \&$sym;
        }

    [download]
  10. require doesn't import; hence 
                eval { require LWP::UserAgent; };
            if ($@) {
                push(@errors, "Failure loading LWP::UserAgent: $@");
                next;
            }

    [download]
    Actually, I prefer this idiom: 
                eval { require LWP::UserAgent }
              or do {
                      push @errors,
                        "Failure loading LWP::UserAgent: $@";
                      next;
                    };

    [download]

Update: In point 6, I changed a $sym =~ /^auto$/ to $sym eq 'auto'. What was I thinking?

the lowliest monk

In reply to Re: New CPAN Module: Math::Random::MT::Auto by tlm
in thread New CPAN Module: Math::Random::MT::Auto by jdhedden

Title:
Use:  <p> text here (a paragraph) </p>
and:  <code> code here </code>
to format your post, it's "PerlMonks-approved HTML":


  • Posts are HTML formatted. Put <p> </p> tags around your paragraphs. Put <code> </code> tags around your code and data!
  • Titles consisting of a single word are discouraged, and in most cases are disallowed outright.
  • Read Where should I post X? if you're not absolutely sure you're posting in the right place.
  • Please read these before you post! —
  • Posts may use any of the Perl Monks Approved HTML tags:
    a, abbr, b, big, blockquote, br, caption, center, col, colgroup, dd, del, details, div, dl, dt, em, font, h1, h2, h3, h4, h5, h6, hr, i, ins, li, ol, p, pre, readmore, small, span, spoiler, strike, strong, sub, summary, sup, table, tbody, td, tfoot, th, thead, tr, tt, u, ul, wbr
  • You may need to use entities for some characters, as follows. (Exception: Within code tags, you can put the characters literally.)
            For:     Use:
    & &amp;
    < &lt;
    > &gt;
    [ &#91;
    ] &#93;
  • Link using PerlMonks shortcuts! What shortcuts can I use for linking?
  • See Writeup Formatting Tips and other pages linked from there for more info.