CODEWITHSUNDEEP
javarecursionrandom
Keerthivasan
Keerthivasan

Reputation: 12890

Bug in recursive function

I'm trying to get a pin number with only 4 digits. But the numbers that are less than 1000 are also getting printed. What is happening in this code?

import java.util.Random;

public class Util {

    public static int getRandomPINNumber(){
        Random randomGenerator = new Random();
        int randomNo = randomGenerator.nextInt(10000);
        if(randomNo < 1000)
        getRandomPINNumber(); //repeat if it is less than 1000,some bug here
        return randomNo;
    }

    public static void main(String[] args){
        for(int i = 0;i<100; i++)
            System.out.println(getRandomPINNumber());
    }

}

Output

6413
1692
5734
105  <--- why 105 is getting printed, it should reevaluate the pin right? 
4857
6348
1355

Upvotes: 4

Views: 564

Answers (5)

Keerthivasan
Keerthivasan

Reputation: 12890

I made the code this way,

public class Util {

    static Random randomGenerator = new Random();

    public static int getRandomPINNumber(){
        int randomNo = randomGenerator.nextInt(9000) + 1000;
        return randomNo;
    }


    public static void main(String[] args){
        for(int i = 0;i<100; i++)
            System.out.println(getRandomPINNumber());
    }

}

Posting the fully bug free code following all the information / tips from other answers

Upvotes: 0

samhareem
samhareem

Reputation: 136

You need to enclose the code you want to process after the if statement in curly braces and assign the result of the method to randomNo.

int randomNo = randomGenerator.nextInt(10000);
if (randomNo < 1000) {
    randomNo = getRandomPINNumber(); //repeat if it is less than 1000,some bug here
}
return randomNo;

You could also avoid values that are < 1000 by using 

int randomNo = randomGenerator.nextInt(9000) + 1000;

This will return numbers from 1000 to 9999 and is a much cleaner solution than recursion.

Upvotes: 2

Denis Lukenich
Denis Lukenich

Reputation: 3174

Just a hint, actually you don't need to use recursion here:

    return randomGenerator.nextInt(9000) + 1000;

is a much more simple solution.

Upvotes: 2

Tunaki
Tunaki

Reputation: 137289

The problem is that you are not returning the result of the recursive call. Change you code to:

public static int getRandomPINNumber(){
    Random randomGenerator = new Random();
    int randomNo = randomGenerator.nextInt(10000);
    if(randomNo < 1000)
        return getRandomPINNumber(); //repeat if it is less than 1000
    return randomNo;
}

When you call the function for the first time and a number less than 1000 is generated, you recursively call getRandomPINNumber but ignore the return value.

Also, you should not call new Random() multiple times. Call it once and store the result.

Upvotes: 8

Bathsheba
Bathsheba

Reputation: 234875

Three things, in order of increasing pedantry

  1. You need to return the value of the recursive function: return getRandomPINNumber(); else you're discarding the result.

  2. Don't call Random randomGenerator = new Random(); multiple times, else you ruin the statistical properties of the generator. Have randomGenerator as a field of your class, or pass it in to your function.

  3. Never discard results of a random number generator else you will introduce some statistical bias. (Over the years very many research papers have been debunked due to improper random number generator usage). In your case you can use int randomNo = randomGenerator.nextInt(9000) + 1000; and drop the discarding altogether.

Upvotes: 3

Related Questions