How can I fix my code that's supposed to give me an array of random integers?-CodePudding

So I started a method which is supposed to give me an array filled with random non repeating values, that additionally are odd. However, I only get the array filled with somewhat wrong values:

public static int[] Array(int n, int max) {
  int [] arr = new int [n];
  int newnum;
  Random rn = new Random();

  for(int i = 0; i < arr.length; i  ){
    newnum = rn.nextInt(0,max);
    for(int j = 0; j < i; j  ){
      while(newnum == arr[j] && newnum % 2 == 0){
         newnum = rn.nextInt(0, max);
        }
      }
   arr[i] = newnum;
    }
return arr;
}

CodePudding user response：

Why not create a set and keep adding till you have n elements?

CodePudding user response：

The issue here is this:

You go through all the numbers already in the list and you reroll your number if it's equal to j. However, once you have done the comparison, your new number could be equal to arr[j-1], you don't re-check. You need to start over every time there is a match.
It should be || and not && in your condition. You want to reroll if the number is equal to a previous number OR if it is even.

The optimisation also is terrible but I guess that's not what you asked.

Fixed version (but not optimized):

public static int[] array(int n, int max) {
  int[] arr = new int[n];
  int newnum;
  Random rn = new Random();

  for (int i = 0; i < arr.length; i  ) {
    newnum = rn.nextInt(max);
    for (int j = 0; j < i; j  ) {
      if (newnum == arr[j] || newnum % 2 == 0) { 
        //or instead of and
        //we are also using if here
        //because we are already re-running the loop by starting over from j = 0
        newnum = rn.nextInt(max);
        j = 0;
      }
    }
    arr[i] = newnum;
  }
  return arr;
}

There are much better ways to do this using java features but I imagine you are still in school and they didn't teach them yet.

CodePudding user response：

You want non-repeating odd numbers in a range. For non-repeating numbers you are better off using a shuffle for a small range or a set for a large range.

For picking odd numbers it might be easier to pick integers from half the range and use num = 2 * pick 1 so as to not generate any even numbers, though you will need to be careful about setting the correct range of integers for the initial pick.

CodePudding user response：

You should make sure that you lay your code out so that you understand it.

You should make sure that your conditions are correct at all times, and don't hesitate to delegate complexity to other methods.

public static int[] array(int n, int max) {
  int[] arr = new int[n];
  Random rn = new Random();

  for (int i = 0; i < arr.length; i  ) {
    int candidate = rn.nextInt(max);
    while (candidate % 2 == 0 || isContainedIn(candidate, arr, i - 1)) {
      candidate = rn.nextInt(max);
    }
    arr[i] = candidate;
  }
  return arr;
}
private static boolean isContainedIn(int candidate, int[] arr, int index) {
  for (int i = 0; i <= index; i  ) {
    if (arr[i] == candidate) {
      return true;
    }
  }
  return false;
}

An alternative is to use streams:

public static int[] array(int n, int max) {
  max /= 2;
  if (n > max) throw new IllegalArgumentException("Impossible to generate such an array");
  int[] array = new Random().ints(0, max) // Generate ints between 0 and half max
    .map(i -> i * 2   1)                      // Make sure they're odds
    .distinct()                               // Make sure they don't repeat
    .limit(n)                                 // Take n of those numbers
    .toArray();                               // Put them in an array
  return array;
}