Commons Math
  1. Commons Math
  2. MATH-1012

Reduce code duplication in "RandomDataGenerator"

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 3.2
    • Fix Version/s: 3.3
    • Labels:

      Description

      Some duplication exists in "o.a.c.m.random.RandomDataGenerator".

      That partly arises from having a method that requires a "o.a.c.m.random.RandomGenerator" parameter vs a similar method but with a "java.security.SecureRandom" parameter.

        Activity

        Hide
        Gilles added a comment -

        Duplicate code removed in revision 1509237.

        Show
        Gilles added a comment - Duplicate code removed in revision 1509237.
        Hide
        Gilles added a comment -

        "RandomGeneratorFactory" class created in revision 1509222.

        Show
        Gilles added a comment - "RandomGeneratorFactory" class created in revision 1509222.
        Hide
        Phil Steitz added a comment -

        I like the idea here. This is why we defined RandomGenerator in the first place, but this code predates it. An alternative to RandomUtils might be to follow the pattern of JDKRandomGenertor - explicitly subclass SecureRandom and implement only the needed methods. I guess you then still have the factory problem to deal with then, so could be you end up having to wrap in any case. So I am OK with the RandomUtils setup, but we should call it something else - e.g. RandomGeneratorFactory or Random2RandomGenerator.

        Show
        Phil Steitz added a comment - I like the idea here. This is why we defined RandomGenerator in the first place, but this code predates it. An alternative to RandomUtils might be to follow the pattern of JDKRandomGenertor - explicitly subclass SecureRandom and implement only the needed methods. I guess you then still have the factory problem to deal with then, so could be you end up having to wrap in any case. So I am OK with the RandomUtils setup, but we should call it something else - e.g. RandomGeneratorFactory or Random2RandomGenerator.
        Hide
        Gilles added a comment -

        The attached patch would allow to remove some duplicated code.

        In summary, a new "RandomUtils" utility class provides a factory method to wrap a "java.util.Random" (which is the parent class of "java.security.SecureRandom") into a CM's "RandomGenerator".
        Then, in "RandomDataGenerator", only one method (that takes a "RandomGenerator" parameter) is necessary.

        The patch passes all the "RandomDataGenerator" unit tests with no change and affects only private methods (backwards-compatible).

        Show
        Gilles added a comment - The attached patch would allow to remove some duplicated code. In summary, a new "RandomUtils" utility class provides a factory method to wrap a "java.util.Random" (which is the parent class of "java.security.SecureRandom") into a CM's "RandomGenerator". Then, in "RandomDataGenerator", only one method (that takes a "RandomGenerator" parameter) is necessary. The patch passes all the "RandomDataGenerator" unit tests with no change and affects only private methods (backwards-compatible).

          People

          • Assignee:
            Gilles
            Reporter:
            Gilles
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development