Debugging..wondering why this will run sometimes but errors the other times

[copelandtml]

public class C_C_ShuffleCards 
{
	public static void main(String[] args) 
	{
		//program name
		System.out.println("The Cards Shuffler");
		
		//What the program does
		System.out.println("\nThis program creates a deck of 52 cards.");
		System.out.println("The cards are represented by the values 1 to 52");
		System.out.println("It then 'shuffles' the cards.");
		
		//create array
		int[] cardsArray = new int[52];
		
		//fill array
		for(int index = 1; index < cardsArray.length - 1; index++)
		{
			cardsArray[index] = (int) ((52-1+1) * Math.random() + 1);
		}
		
		//printout of array before being shuffled
		System.out.println("\nThe contents of the array before shuffling...");
		printArrayValues(cardsArray);
		
		//shuffle cards
			//use for loop to loop through array
			for(int index = 1; index < cardsArray.length - 1; index++)
			{
				//generate random number
				int randomNumber = (int) ((52-1+1) * Math.random() + 1);
			
				//compare adjacent element values 
				if(cardsArray[index] != cardsArray[index + 1])
				{
					//call the swapArrayElements() method
					swapArrayElements(cardsArray, index, randomNumber);
					
				}//end if
				
			}//end for loop
			
			//print out of array after being shuffled
			System.out.println("\n\nThe contents of the deck after shuffling...");
			printArrayValues(cardsArray);
	
	}//end main
	
	
	/*
	 * Method Name: printArrayValues()
	 * Purpose: prints to the screen the values in an int array
	 * Accepts: an array of type int
	 * Returns: NOTHING! IT'S A VOID METHOD!
	 */
	public static void printArrayValues(int [] array)
	{
		//set up a for loop
		for(int index = 0; index < array.length; index++)
		{
			//set up an if statement so that whenever the index is a multiple of 10 it does a line feed
			if(index % 10 == 0)
			{
				System.out.println();//prints a blank line
			}
			
			System.out.print(array[index] +", ");
		}//end for
	}//end method
	
	
	/*
	 * Method Name: swapArrayElements()
	 * Purpose: swaps the values between the two specified elements of an array of type int
	 * Accepts: one array of type int, and two int values which represent the index numbers of the two array elements involved in the swap.
	 * Returns: NOTHING! It's a void method.
	 * */
	
	public static void swapArrayElements(int[]array, int index1, int index2)
	{
		//do the swap using a temp variable
		int temp;//my empty glass
		//do the swap
		temp = array[index1];
		array[index1] = array[index2];
		//complete the swap
		array[index2] = temp;
		
	}//end method

	
}//end class

It will run fine in Eclipse every once and a while and other times it will error out with the following errors:

Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException: 52
at C_C_ShuffleCards.swapArrayElements(C_C_ShuffleCard s.java:92)
at C_C_ShuffleCards.main(C_C_ShuffleCards.java:44)

[berry120]

It seems to me you don't quite understand how array indices work which is causing the problem here. There's actually two issues that I can see straight off, but only one is causing the exception, one is a more subtle bug.

If you create an array of size n, the valid indices are 0..n-1. So given an array called foo of size 52 (which is what you have here), foo[0] is valid, foo[51] is valid (and everywhere in between) but foo[52] will give you an IndexOutOfBoundsException.

The bad line is here:

int randomNumber = (int) ((52-1+1) * Math.random() + 1);

First off, why is the -1+1 there? You can get rid of that straight away without any difference in the result! Secondly, Math.random() will give a result between 0 and 1 (though it will never be 1, it's an exclusive boundary.) Multiplication takes precedence, so the result is 52*a number between 0 and 1. Let's say the random number was very close to 1 and the result is thus 51.9 (a perfectly valid result.) 1 will be added, giving us 52.9. We then cast to an int (which always just drops the decimal component, it doesn't round) giving us 52, an invalid index. What you actually want here if you're using Math.random() is simply:

int randomNumber = (int) (52 * Math.random());

However, strictly speaking it's better practice to use the Random class, which performs better when you're just using it for integers.

Secondly:

for(int index = 1; index < cardsArray.length - 1; index++)

This will loop between 1 and 50, thus missing out the first and last elements in the array. This:

for(int index = 0; index < cardsArray.length; index++)

Is probably what you want instead.

[copelandtml]

Thanks for the quick reply, and the brief explanation on Math.random and arrays, I fixed the two lines where I was using Math.random and corrected the two for loops.

And then noticed my if statement was wrong

if(cardsArray[index] != cardsArray[index + 1])

I changed it too

if(cardsArray[index] == cardsArray[index] || cardsArray[index] != cardsArray[index])

Seem to be working great now.

[berry120]

if(cardsArray[index] == cardsArray[index] || cardsArray[index] != cardsArray[index])

Umm - that will always, always return true. The second part of the OR condition won't even get evaluated, since you're comparing whether one value is equal to itself... which will always return true.

What are you trying to achieve with that if statement? What's it meant to be doing? If it definitely works as is then you can just delete that line because it won't change anything in the flow of the code!

[copelandtml]

OR statement is now gone and still works great. Thanks again, I'm new to this still as you can tell. More of just a logic error on my part and not totally understanding the concept at hand yet, but thanks for all the info. Take care!