5
\$\begingroup\$

In the code below, the primary purpose of class MyRNG is to create a single method getMyRandom() that will return a random number from any of several very different distributions and generators. The actual distribution and generator will be determined by information read from a file, although in the test program below, the information is simply stated in main() at the point where the call to MyRNG() occurs. In the code as it stands, a line (read from a file, console or command line) is parsed to extract the type of RNG wanted and the parameters for the distribution. Distributions are limited in the sample code to a Gaussian distribution, uniform distribution, and random choice

I think that the code is horrible (not surprising given that I'm an OOP novice) in that I use an enum to signal which kind of RNG I really want, and then I use switch statements to ensure that getMyRandom() actually calls the correct kind of basic generator. I think I should be able to solve in another way! I have been reading about design patterns and I've convinced myself that it should be possible to greatly improve the code by using either Factory Method or Abstract Factory but I cannot see how to do it.

Questions that I'm struggling with ...

  • What design pattern should I be focusing on?
  • Most importantly for my understanding, what would a sketch of that design pattern look like in the context of my problem
  • At the moment I have "this.rng = new Random()" within class MyRNG class where (I believe) it is called for each new MyRNG object. Does this make sense or should it be somewhere else, either in the code as it stands or in a relevant design pattern.
  • Where should the parsing of the RNG description go?
import java.util.Random;

class MyRNG {

    // The DistributionType enum is used to keep track of the particular kind of RNG that
    // we want so that the appropriate specific method can be called by the intermediary 
    // getMyRandom() method.
    enum DistributionType {
        UNIFORMDOUBLE, // Types of distribution to account for. ** VERY **Incomplete list.
        GAUSSIAN,
        CHOICE
    };

    Random rng;
    DistributionType iAmThisTypeOfRNG; // Keeps track of what kind of RNG is ultimately called
    String description;
    
    
    // Constructor
    public MyRNG (String description) {
        
        this.rng = new Random();
        this.description = description;
        
        
        // Parse the description to determine what kind of RNG is really wanted
        if (description.matches("UNIFORMDOUBLE +-?\\d+\\.?\\d+ +-?\\d+\\.?\\d+")) {
            // For doubles on a unform distribution use UNIFORMDOUBLE lowerbound upperbound
            // ... UNIFORMDOUBLE 18.3 22.2 
            this.iAmThisTypeOfRNG = DistributionType.UNIFORMDOUBLE;
        } else if (description.matches("GAUSSIAN +\\d+\\.?\\d+ +\\d+\\.?\\d+")) {
            // For Gaussian distribution use GAUSSIAN mean std
            // ... GAUSSIAN 2.0 3.5 
            this.iAmThisTypeOfRNG = DistributionType.GAUSSIAN;
        } else if  (description.matches("CHOICE -?\\d+\\.?\\d+( +-?\\d+\\.?\\d+)*")) {
            // For CHOICE distribution use CHOICE n1 n2 ...
            // ... CHOICE 4.6 -2.35 1.8 -4.0 -1.9
            this.iAmThisTypeOfRNG = DistributionType.CHOICE;
        } else {
            System.out.println("Error: Could not parse parameter string: " + description);
        }
    }
    
    public double getMyRandom() {
        
        double myRand;
        double[] parameters = parametersFromDescription(description);
        
        switch (iAmThisTypeOfRNG) {
        case UNIFORMDOUBLE: // double on uniform distribution
            myRand = myNextUniformDouble(rng, parameters);
            // Next line for debugging info only
            System.out.println("Parsed UNIFORMDOUBLE with description: " + description);
            break;
            
        case GAUSSIAN: // doubles from Gaussian distribution
            myRand = myNextGaussian(rng, parameters);
            // Next line for debugging info only
            System.out.println("Parsed GAUSSIAN with description: " + description);
            break;
            
        case CHOICE: // Random choice from a list of doubles
            myRand = myNextChoice(rng, parameters);
            // Next line for debugging info only
            System.out.println("Parsed CHOICE with description: " + description);
            break;
            
        default:
            myRand = 0;
            System.out.println("Non-existent kind of random string requested.");
            
        }
        return myRand;
    }
    
    
    public String toString() {
        return "This MyRNG object has kind: " + iAmThisTypeOfRNG;
    }
    
    static double[] parametersFromDescription(String description) {
        
        String[] parameterString = description.split("\s+");
        double [] parameters = new double[parameterString.length - 1];
        
        
        int j = 0;
        for (int i = 1; i < parameterString.length; i++) {
            parameters[j] = Double.parseDouble(parameterString[i]);
            j++;
        }
        return parameters;      
    }
    
    static double myNextUniformDouble(Random rng, double[] parameters) {

        double lowerBound = parameters[0];
        double upperBound = parameters[1];
        
        return (upperBound-lowerBound) * rng.nextDouble() + lowerBound;
    }
    
    static double myNextChoice(Random rng, double[] parameters) {

        int selection = rng.nextInt(parameters.length);
        
        return parameters[selection];
    }
    
    static double myNextGaussian(Random rng, double[] parameters) {

        double mean = parameters[0];
        double standardDeviation = parameters[1];
        
        return standardDeviation * rng.nextGaussian() + mean;
    }
}
    

public class Main {
    
    public static void main(String[] args) {
        
        MyRNG rsA = new MyRNG("GAUSSIAN 5.66 1.2");

        System.out.println(rsA);
        System.out.println("Result of getMyRandom on rsA is : " + rsA.getMyRandom());
        System.out.println();

        MyRNG rsB = new MyRNG("UNIFORMDOUBLE -2.0 2.0");

        System.out.println(rsB);
        System.out.println("Result of getMyRandom on rsB is : " + rsB.getMyRandom());
        System.out.println();

        MyRNG rsC = new MyRNG("CHOICE 2.0 4.0 6.0 8.0 10.0 12.0");

        System.out.println(rsC);
        System.out.println("Result of getMyRandom on rsC is : " + rsC.getMyRandom());
        System.out.println(rsC.getMyRandom());
    
     }
}
\$\endgroup\$
0
9
\$\begingroup\$

A lot of the time, if you want to group some objects by a type, the cleanest approach is... well, making types. Classes.

It seems natural to me to start without the enum, instead beginning with MyRNG as an interface. The MyRNG type doesn't know anything about generating random numbers, but it can still demand that any object belonging to that type does know how to generate random numbers. Then you can easily add various distributions like

class GaussianRNG implements MyRNG {
    private final double mean;
    private final double standardDeviation;
    private final Random rng = new Random();

    public GaussianRNG(double mean, double standardDeviation) {
        this.mean = mean;
        this.standardDeviation = standardDeviation;
    }

    @Override
    public double getMyRandom() {
        return standardDeviation * rng.nextGaussian() + mean;
    }
}

And thanks to subtyping, you can just go MyRNG rng = new GaussianRNG(5.66, 1.2);, even though MyRNG has no idea GaussianRNG exists! And when you want another distribution, you just add another subtype, and nobody necessarily needs updating.

Now, you'll notice I kind of dropped the ability to pick a distribution type at runtime. But there's nothing saying you can't have that too! You're definitely allowed to have factory methods à la

public static MyRNG createRNG(DistributionType distribution, double[] parameters) {
    switch (distribution) {
        case UNIFORM_DOUBLE:
            if (parameters.length != 2) throw new IllegalArgumentException();
            return new UniformDoubleRNG(parameters[0], parameters[1]);
        case GAUSSIAN:
            if (parameters.length != 2) throw new IllegalArgumentException();
            return new GaussianRNG(parameters[0], parameters[1]);
        case CHOICE:
            return new ChoiceRNG(parameters);
    }
}

or a variant of that which parses a String. Or both! After all, if you have the string parsing and you have that createRNG, connecting the two together should be simple:

public static MyRNG createRNG(String description) {
    DistributionType distributionType = typeFromDescription(description);
    double[] parameters = parametersFromDescription(description);
    return createRNG(distributionType, parameters);
}

Note that the DistributionType never makes it into the MyRNGs themselves - they don't need an external enum to tell them what their type is, because information about their type is contained in, well, their type.

Finally, I would like to point out a few more things about the way your current approach is implemented:

  • It seems a bit strange to me that you call parametersFromDescription each time you fetch a random number. Wouldn't it be easier to just do that once during construction and store the parameters? That way you can even check during construction to make sure they're valid.
  • On a related note, the MyRNG constructor writes an error message to System.out... but it doesn't actually tell the program that anything went wrong. It returns just fine, and whoever called it ends up with a MyRNG object and will probably assume it's valid and usable. Instead of printing an error message (which might not be something you want to do!) you should throw an exception of some sort to let the caller know that it isn't going to get anything sensible and it should figure out a way to deal with that (which might mean just displaying an error message to the user). Same for getMyRandom, its job isn't to display error messages to the user, its job is to return a random number - if for some reason it can't, it should make it obvious that it failed so the caller can handle it in whatever manner is appropriate. throwing exceptions is a good way to do that.
  • I don't see why the myNextWhatever methods are static. If they weren't, you wouldn't need to pass the Random as a parameter since you'd have access to this.rng. And perhaps this.parameters if you were to save the parameters as well
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Many thanks Sara. I have yet to try out all the suggestions that you have made but I shall. I really appreciate the time you've taken for the very extensive comments and sample code. One of the things that your example immediately highlighted was a place where I've come unstuck. I hadn't understood that an interface can not only be implemented but that a new object can be created as a member of the interface. You have, for example "MyRNG rng = new GaussianRNG(5.66, 1.2);" I'd looked at an example here ( w3schools.com/java/java_interface.asp ) ... continued. \$\endgroup\$
    – user02814
    Jun 25 at 8:36
  • 2
    \$\begingroup\$ But in in the w3schools example, despite an interface Animal being defined and implemented by class Pig ... when it comes to creating the Pig, the example actually makes a call to Pig, as in Pig myPig = new Pig(); . It wasn't clear to me that the example could have used a generic Animal which then happened to be a Pig ... as in Animal myAnimal = new Pig(); The thing I'll do now is work steadily through your example and then perhaps return to codereview later. \$\endgroup\$
    – user02814
    Jun 25 at 8:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.