ArrayIndexOutOfBoundsException

[Caiz Austin]

This code generates random non-duplicate integers, stores a number into a randomPosition int, shuffles an array of briefcase values, then stores a random value into a briefcase object. When I run the code, I get an error right after the random number "26" has been obtained and stored into randomPosition from "randomPosition = list.get(i). However it says that the error is coming from line 36. Does anyone know what I need to fix to get this to work?

public static int getRandomInt(int min, int max) {

        return random.nextInt((max - min) + 1) + min;
    }

    public static ArrayList<Integer> getRandomIntegers(int size, int min,int max) {
        
        ArrayList<Integer> number = new ArrayList<Integer>();

        while (number.size() < size) {
            int random = getRandomInt(min, max);

            if (!number.contains(random)) {
                number.add(random);
            }
        }

        return number;
    }

  public static void main(String args[]) { 

ArrayList<Integer> list = getRandomIntegers(26, 1, 26);
    
     for (int i = 0; i < list.size(); i++) {
         System.out.println(list.get(i));
      
            System.out.println("" + list.get(i));
            randomPosition = list.get(i);

            //shuffle case values
        temp = valuesRandomised[i];
        valuesRandomised[i] = valuesRandomised[randomPosition];
        valuesRandomised[randomPosition] = temp;

        briefcases[i] = new Briefcase(valuesRandomised[i], false);
        System.out.println("Case " + (i + 1) + ": " + briefcases[i].getValue());
    }

[Campbell Ritchie]

What does 26 mean? Look at line 23. As you will see from the old Sun style suggestions §10.3, it is not a good idea to have number literals floating around without explanation. It is usually better to declare such numbers as constants:-

class Foo
{
   private static final int LETTERS_IN_ALPHABET = 26;
   // ...
}

Then everybody knows what each means, and also whether the two 26s both mean the same thing.
What does the error message say? Does it say 26 anywhere? Does that List have 26 elements? Does the array have a length of 26? As you doubtless already know, the indices of a 26‑element List or array run 0…25, so you can expect an index out of bounds exception if you look for index 26. I would say inspect your loop to see what numbers you are getting to, but that code still looks as if somebody had been guessing. You can make 1,000,000 guesses and there is a very good chance that one will be correct. Or you can think about the code, plan and design it and the first attempt will be correct.
You also seem to be doing too much all in the same place. What does that code do? If you write down more than one thing, that means it should be broken up into multiple methods, each doing one thing. You are going to confuse yourself by having so much all happening together. It is also much more difficult to work out what is going wrong. If you look here, you can find out hoow code to swap two elements in an array doesn't only belong in a different method but belongs in a different class.

[Lakshman Arun]

valuesRandomised size could be 26, ie it has elements from 0 to 25, not 26th

[Liutauras Vilda]

We have seen some portion of this code in other your thread. Could you please copy and paste full code with the right formatting?

I find your code difficult to read, don't know exactly why. Formatting most likely is the first in the queue. Noise of comments is another.

1. If your intention is not to have duplicate values in the data structure - use right data structure which ensures that. If insertion order matters, use data structure which ensures there are no duplicates + which maintains insertion order. If in the final end such data structure needs to be List, convert it to List after you ensured there are no duplicates and order maintained. That way your intention would be much clearer rather than check if List contains element or not and then decide whether add such element or not.

So, this:

ArrayList<Integer> number = new ArrayList<Integer>();

// ...
if (!number.contains(random)) {
    number.add(random);
}

Change to:

Set<Integer> numbers = // figure out which to initialize
// ...
numbers.add(random);

// convert to List using streams

2. Don't name variables as "list". At most it tells you about implementation details and nothing else. You need more than that, in fact implementation details you don't need to know. You need to know, that "list" holds random integers, so call it "randomIntegers" - much better, isn't it?

3. Avoid writing comments which tell nothing. This code snippet contains only 1 comment, but in your other thread code contained lots of comments which were all less of a use and misleading.

4. Formatting formatting formatting. Important!


For the question at hand, what is the briefcases array length? It might can't hold so many elements as you are trying to add.

[Caiz Austin]

Thanks for your replies I really appreciate it! I will continue to work on how I format my code. Liutauras, do you think it has something to do with how I have included "{" in the same line as the method declaration? Originally I would put "{" on a separate line below by itself.

Anyway, this is what I have used:

    public static HashSet<Integer> getRandomIntegers(int size, int min, int max) {

        HashSet<Integer> number = new HashSet<Integer>();

        while (number.size() < size) {
            int random = getRandomInt(min, max);
            number.add(random);
        }

        return number;
    }

    public static void main(String args[]) {

        Set<Integer> intSet = new HashSet<Integer>(getRandomIntegers(26, 0, 25));

        List randNumbers = intSet.stream().collect(Collectors.toList());
        
        for (int i = 0; i < randNumbers.size(); i++) {

            randomPosition = (int)randNumbers.get(i);

            briefcases[i] = new Briefcase(values[randomPosition], false);
        }
}

I realised it was unnecessary to use [temp] to shuffle the values in the array AS WELL as obtaining random numbers. I have removed this and now "get" the index of a random element in the array which works

[Carey Brown]

This code generates random non-duplicate integers...

If you need 26 unique random numbers whose values are 0 through 25(inclusive), then you essentially have a list of 0 through 25 that has been shuffled (i.e. sorted in random order). If the HashSet size < the number of possible int values then your approach makes sense. If HashSet size is exactly equal to the number of possible int values then that's the wrong way to go about it. Let's say you've managed to fill the HashSet with 25 unique values and there's only one slot left to fill. Now you know that there's only one specific value that would be unique from all the others, yet you sit in a loop waiting until you get a random number that is that missing value.

So, alternatively, create a list of 26 int values and initialize them to the values 0 through 25, in order. Then shuffle the list to randomize them.

Collections.shuffle( randomNumbers );