Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

There is my SSCCE to generate a value of discrete random variable.

values is set of value the RV can take and procents is equivalent to discrete pdf.

Can you anticipate any issue with this snippet?

import java.util.Random;

public class RandomTest {
    public static void main (String[] args) throws Exception {
        RandomTest rt = new RandomTest();

        int[] values = {0, 1, 2};
        int[] procents = {30, 60, 10};

        for (int i=0; i < 10; i++) {
            System.out.print(rt.discreteRV(values, procents) + " ");
        }
    }

    public int discreteRV (int[] values, int[] procents) throws Exception {
        if (values == null || procents == null) throw new Exception("Input parameters are null");
        if (values.length != procents.length) throw new Exception("Input parameters length mismatch");

        int sumProcents = 0;
        for (int i=0; i < procents.length; i++) {
            if (procents[i] < 0) throw new Exception("Negative procents are not allowed");
            sumProcents += procents[i];
        }

        if (sumProcents != 100) throw new Exception("Sum of procents is not 100");

        int rand = new Random().nextInt(100);

        int left = 0, right = 0;
        for (int i=0; i < procents.length; i++) {
            right += procents[i];
            if (rand >= left && rand < right)
                return values[i];
            left = right;
        }

        throw new Exception("");
    }
}
share|improve this question
add comment

1 Answer 1

up vote 1 down vote accepted

In general, this looks ok, i.e. it works.
I will put some notes on the lines of your method and will provide my suggestion after this.

Spelling: procent is not an English word. Well, the compiler does not care, but the next programmer probably will.

public int discreteRV(final int[] values, final int[] procents) throws Exception {

To throw a exception of type Exception does not provide any help. The purpose of exceptions is to communicate errors. Noone knows the type of error from Exception and can do anything about it. They should be avoided in throw statements.

    if (values == null || procents == null)
        throw new Exception("Input parameters are null");

While checking your contract is a good idea, you do not gain anything here.
Java will throw a Nullpointer error with nearly the exact same message anyway if you try to access them.
As long as there are no plans to do any special things, I would not waste lines on this.

    if (values.length != procents.length)
        throw new Exception("Input parameters length mismatch");

I would throw a runtime exception. IllegalArgumentException looks suitable here.

    int sumProcents = 0;
    for (final int procent : procents) {
        if (procent < 0)
            throw new Exception("Negative procents are not allowed");
        sumProcents += procent;
    }

For exception: same as above.
For check: This is not valid for all input.
Consider (and/or try:) final int[] procents = { 1234567890, 1234567890, 1825831616 };
If you want to check the input, check for both sides.

    if (sumProcents != 100)
        throw new Exception("Sum of procents is not 100");

For exception: same as above.

    final int rand = new Random().nextInt(100);

If you use this method frequently, make it static (and/or even ThreadLocalRandom)

    int left = 0, right = 0;
    for (int i = 0; i < procents.length; i++) {
        right += procents[i];
        if (rand >= left && rand < right)
            return values[i];
        left = right;
    }

The left check is not needed.
If you check for something in between, support your readers:
Instead of rand >= left && rand < right try to read left <= rand && rand < right

    throw new Exception("");

For exception: same as above. A IllegalStateException looks suitable here.

}

Suggestion:

public int discreteRV(final int[] values, final int[] percentages) {
    if (values.length != percentages.length)
        throw new IllegalArgumentException("values.length != percentages.length");

    int sumPercentages = 0;
    for (int i = 0; i < percentages.length; ++i) {
        if (percentages[i] < 0)
            throw new IllegalArgumentException("Negative percentages are not allowed: percentages[" + i + "] = " + percentages[i]);
        sumPercentages += percentages[i];
        if (sumPercentages > 100)
            throw new IllegalArgumentException("Sum > 100");
    }

    if (sumPercentages != 100)
        throw new IllegalArgumentException("Sum of percentages is not 100");

    final int randomIntUpTo100 = random.nextInt(100);
    int threshold = 0;
    for (int i = 0; i < percentages.length; i++) {
        threshold += percentages[i];
        if (randomIntUpTo100 < threshold)
            return values[i];
    }
    throw new IllegalStateException("No value found. rand:" + randomIntUpTo100);
}

More ideas:
The arguments (arrays) could be replaced by an Object. This could make the handling easier. Depends on the use case.
The two loops could be combined into one loop. You will loose the check for sum == 100 then.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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