CODEWITHSUNDEEP
javaarraysrandomrepeat
FishyK
FishyK

Reputation: 169

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

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;
}

Upvotes: 1

Views: 511

Answers (5)

Valerij Dobler
Valerij Dobler

Reputation: 2804

As Janardhan Maithil suggested you could utilize a set for the destinctveness

But you also have to take into account what should happen if one would try to get more values, than are available.

public static int[] randomDistinctiveOdds(int maxAmount, int maxValue) {
    if (maxAmount / 2 > maxValue) {
        throw new IllegalArgumentException("Not enough distinct odd values in the range from 0 to " + maxValue);
    }

    Random random = new Random();
    Set<Integer> result = new HashSet<>(maxAmount);

    int nextInt;
    while (result.size() < maxAmount) {
        if ((nextInt = random.nextInt(maxValue)) % 2 == 1)
            result.add(nextInt);
    }

    return result.stream().mapToInt(Integer::intValue).toArray();
}

or a solution with streams

public static int[] randomDistinctiveOddsWithStreams(int maxAmount, int maxValue) {
    if (maxAmount / 2 > maxValue) {
        throw new IllegalArgumentException("Not enough distinct odd values in the range from 0 to " + maxValue);
    }

    List<Integer> odds = IntStream.range(0, maxValue)
            .boxed()
            .filter(i -> i % 2 == 1) // only odds
            .collect(Collectors.toList());

    Collections.shuffle(odds);

    return odds.stream()
            .mapToInt(Integer::intValue)
            .limit(maxAmount) // take at most maxAmount
            .toArray();
}

Upvotes: 1

Olivier Gr&#233;goire
Olivier Gr&#233;goire

Reputation: 35477

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; // Divide by two to avoid generating twice the expected numbers.
  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;
}

Upvotes: 2

rossum
rossum

Reputation: 15683

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.

Upvotes: 1

jeekiii
jeekiii

Reputation: 296

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.

Upvotes: 2

Janardhan Maithil
Janardhan Maithil

Reputation: 81

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

Upvotes: 1

Related Questions