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.

I have this class which is used as part of a game. It needs to generate Random Even values, which is done by generating random numbers until the result is even.

Is there a better way to do this?

Also, I currently have the methods implemented as private instance methods, but should I declare generateRandomNumber() and evenNumber(number) as static methods? Will it have any benefits?

public class Game {
    //...

    public void opponentSaysEvenNumber() {
        int number = generateRandomEvenNumber();
        System.out.println("Opponent: " + number);
    }

    private int generateRandomEvenNumber() {
        Random random = new Random();
        int number = random.nextInt();
        while (!evenNumber(number)) {
            number = random.nextInt();
        }
        return number;
    }

    private boolean evenNumber(int number) {
        return (number % 2) == 0;
    }
}
share|improve this question
16  
Just wondering, why not just multiply whatever number you receive from the RNG by 2? –  Wet Feet Jan 17 at 2:35
4  
Add 1 to the odd ones that is generated? –  PatricK Jan 17 at 6:20
4  
@lohoris Because the 'add 1' operation requires you to test beforehand whether you have an odd or even number, so it's one test and one add operation, or just one test and a null; 'multiply by 2' always yields an even number, so it's always just a single operation that can be reduced to a 'bitwise left shift by 1' at assembly level. I don't think it's possible to get much faster than that... –  Shadur Jan 17 at 9:11
4  
... Except maybe using a bit mask to zero the least significant bit, come to think of it. –  Shadur Jan 17 at 9:13
2  
I also would recommend a left shift operation, and for completeness sake, would like to point out bitwise AND'ing with NOT(1) would work as well (in one operation). –  Boluc Papuccuoglu Jan 17 at 9:14

9 Answers 9

up vote 38 down vote accepted

Yes. I would go one step further and declare your functions as private static final, if possible. The combination of those three keywords means that the code would be unaffected by any instance variable, any superclass, or any subclass, and is also not callable by any code external to the class. Therefore, the compiler has enough of a hint that it could decide to inline the entire function.

I would also rename evenNumber(int number) to isEven(int number). There is a convention in Java that functions named isSomething() return a boolean and have no side effects. Your function meets those criteria.

To generate a random even number, you could just take random.nextInt() & -2 to mask off the least significant digit. That would be more efficient than looping, testing, and discarding. In that case, the whole question about helper functions would be irrelevant.

It's bad practice to create a new instance of Random every time you want to generate one random number, though. The pseudorandom number generator actually carries some state, even if you don't think of it that way. You should therefore use a private static variable to store the Random object.

share|improve this answer
4  
final does nothing to a private static method. –  omiel Jan 16 at 13:43
    
@omiel will the JIT still inline it? –  Cruncher Jan 16 at 14:41
3  
Static methods are not part of the inheritance, so there is no way for another class to over-ride them anyway. That's why final is meaningless. –  Tim B Jan 16 at 14:59
1  
Would there be a reasonable argument that adding final improves future readability? –  StingyJack Jan 16 at 15:37
2  
before using a private static Random variable, consider the following excerpt from the javadoc: Instances of java.util.Random are threadsafe. However, the concurrent use of the same java.util.Random instance across threads may encounter contention and consequent poor performance. Consider instead using ThreadLocalRandom in multithreaded designs. –  Buhb Jan 16 at 23:07

Declaring such methods static can increase the readability of your code. For the reader it will be obvious that the method does not depend on the internal state of an instance of the class.

share|improve this answer
2  
What's more calling static method is a little bit faster (no implicit passing of this to the method) –  Adam Dyga Jan 16 at 10:19
1  
@Adam I'm pretty sure that would get compiled out of the resulting bytecode. –  corsiKa Jan 17 at 0:51

Your technique is inefficient. To generate even numbers, just do any of the following:

  • Mask off the least significant bit (-2 is 0xFFFFFFFE, or a bitmask with all ones except for the last bit)

    random.nextInt() & -2;
    
  • Shift left by one bit using the bit-shift operator

    random.nextInt() << 1;
    
  • Shift left by one bit using multiplication

    random.nextInt() * 2;
    
share|improve this answer
    
Well spotted. At least the op should write if(!evenNumber(number))number++, so he wouldn't generate other random number –  Bruno Costa Jan 17 at 0:35
2  
Using a bit mask is not acceptable because it modifies the number distribution. Ex: random outputs 2,3 become 2,2. Shifting left or multiplying are the only acceptable way. –  Carl Jan 17 at 3:48
4  
@Carl No it doesn't (at least not for positive integers - for negative or positive integers there might be a slight bias around zero, but this doesn't apply here). All even numbers still occur with the same probability (they all can be output by the PRNG outputting themselves, or the number immediately following them). –  Thomas Jan 17 at 4:48
    
There's a little flaw, when you multiply or shift left, you need to keep into account that you are working on integers, not unsigned integers, and a overflow will result in change of sign. –  DarioOO Jan 17 at 9:48
    
@DarioOO Overflow doesn't matter. All three techniques result in 31 random bits followed by a 0 bit. –  200_success Jan 21 at 3:45

Why not just double the first random number generated and return it? You will still have random numbers, their distribution is not corrupted and its faster.

public class Game {
    //...

    public void opponentSaysEvenNumber() {
        int number = generateRandomEvenNumber();
        System.out.println("Opponent: " + number);
    }

    private int generateRandomEvenNumber() {
        Random random = new Random();
        int number = random.nextInt();
        return number * 2;
    }
}
share|improve this answer

If you don't use any of the attributes of the class, then it means that you should probably put this method somewhere else. Maybe in another class, a RandomEvenNumberGenerator for example.

I've always considered static a bad practice. It opens the path to so many problems, semantic issues and "I know where you live" syndrome, which in turns make testing harder, etc.

share|improve this answer
1  
Could you elaborate what the "I know where you live" syndrome is in the context of unit testing? Google did not help me out on this one. –  Andris Jan 16 at 10:44
    
Unit testing is testing each component, each class in total separation from the others, to avoid any incontrollable side effects. Typically, you will have a hard time testing a component that's using a singleton, because you can't easily substitute the singleton with something you control. The component "knows where the singleton lives" and addresses directly to it. The better way would be to provide an instance of the singleton to the object that needs it. In the code given, it wouldn't really hurt, but having static methods encourages this coding style. –  Antoine_935 Jan 16 at 10:59
5  
No, if this method is used in that class by its methods and nowhere else, it should certainly be static private. This makes a lot more sense than having a static class full of disparate helper functions, or a different class for each helper function. Static non-private methods in non-static classes are a different story. Your argument against singletons isn't really applicable here. –  jwg Jan 16 at 15:49
    
"private static" used this way for helper functions is pointless and misleading. It can be considered bad practice. The word "static" adds uneccesary syntactic noise, and it might be taken as a warning that there is some kind of singleton mechanism hidden somewhere. –  Ichthyo Jan 16 at 22:42
    
@jwg, I never advertised the Utility class. Of course not. Having this method as static in this class forces to recreate a new Random instance each time we call the method, or else we need to share it as static property (danger!). I agree with Ichthyo in that it adds unnecessary code noise. –  Antoine_935 Jan 17 at 9:11

You do not want to mask to make your elements even. There are an odd number of positive and an odd number of negative values. This can throw you for a loop (no pun intended) by causing them to be slightly biased.

Instead, you want to provide a min and max and find an even number in that range. You want to do this by shifting (actually division and multiplication, but because we're dealing with a power of 2 we can do it with bit shifts) and not with rounding (which is what masking is essentially).

As a general rule of thumb, you don't want to throw away random bits when you don't have to.

private static final Random rand = new Random(); 

public static int randomEvenNumber(int min, int max) {
    if(min % 2 != 0) throw new IllegalArgumentException("Minimum value must be even");
    if(max % 2 != 0) throw new IllegalArgumentException("Maximum value must be even");
    int range = max - min;
    int rangeHalf = range >> 1; // divided by 2
    int randomValue = rand.nextInt(rangeHalf);
    randomValue = randomValue << 1; // multiplied by 2 to make it even
    return min + randomValue;
}

public static final int randomEvenNumber(int max) {
    return randomEvenNumber(0,max);
}
share|improve this answer
    
To be fair, if you care that much about bias, you probably don't want to use java.util.Random to begin with. This would also be more readable and just as efficient if you did * 2 and / 2 instead of shifting. –  Chris Hayes Jan 17 at 3:17
2  
It depends what you consider to be "fair". Masking off the LSB (or shifting off the MSB) does pick numbers from the set of all representable even ints with uniform probability. However, since there is one more representable negative even number than there are representable positive even numbers, you correctly note that the distribution is not exactly centered at 0. –  200_success Jan 17 at 7:02
    
Instead of throwing exceptions on min and max, why not use the rounded values? –  Henk Langeveld Jan 17 at 12:21
    
@Henek Min and max parameters typically use the following contract: Min is the lowest acceptable value, and max is the first value greater than the highest acceptable value. If min was not even, it would not be an acceptable value which breaks the contract. The case for max is similar. One could argue that those conditions do not belong there. For example, randomEvenNumber(9,19) might have solution set {10,12,14,16,18} - the logic would be slightly more intricate (determine if min is odd and adjust accordingly) but would not involve rounding. –  corsiKa Jan 17 at 16:45

Declaring a static and unique random number generator method is a good choice if you want to centralize the generation of those numbers. However, don't forget to also mark the method as synchronized if you're planning to run your code in a multithreaded environment.

You should also declare the Random instance as private final static.

Finally, as the evenNumber method consists of a single test, you could move that test to your generateRandomEvenNumber method and have your class composed by only two methods.

Another possibility if you want to centralize the generation of those random numbers is to create a Singleton random numbers generator.

share|improve this answer
    
I agree with you insofar the generateRandomEvenNumber should be moved out of that class, since it isn't related to the core concern of this Game class. But I disagree on the inlining of evenNumber. You loose the self-documenting through the name, and you force the user to figure out what the code does. And it would violate the "single level of abstraction" principle. –  Ichthyo Jan 16 at 23:03
    
Nice point about the "single level of abstraction" principle, Jamal, thanks. –  user3067411 Jan 19 at 19:08

Perhaps a higher-level approach may get you to a better solution.

The random generator methods (generateRandomEvenNumber() and evenNumber()) are mathematical in nature and really not specific to the game. They have no dependencies on the game class (e.g. they don't use fields) and they have no side effects.

My approach would be to do something like this. Create a RandomService class with those methods and inject an instance of the service into the game. Using Spring or some other DI (dependency injection) framework, the service can be created as a singleton. The methods on RandomService can be public because they have no dependencies or side effects and they are easily testable, separately from the game.

share|improve this answer

Consider using a ThreadLocalRandom

private int generateRandomEvenNumber() {
    return ThreadLocalRandom.current().nextInt() & -2;
}

ThreadLocalRandom was added in Java 7 and solves a few performances issues with the Random object, especially in multi-threaded environments. Even if you're not in a multi-treaded environment, I think it is a good idea to use it every time. Furthermore the Api is nicer than the old Random one (you don't have to create the object, just use the static method).

Using & -2 (or any other method proposed by @wizzi poo) will ensure the generated number is always even by setting the last bit of the generated number to 0. This is better than doing a while as there is always the chance Random could never generate an even number (highly unlikely).

share|improve this answer

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.