What am I missing here? (ArrayList check)

[Jason Sgalla]

I am working on a simple lotto game where the player enters 10 numbers and is stored in an ArrayList (pNums), the computer generates 20 numbers also stored in an ArrayList (cNums), then finally places the matches into a third ArrayList (mNums). The problem I am having is with checking, removing existing, then replacing the cNums arraylist.

I'm new to ArrayLists so my problem could be with the logic or my usage of ArrayLists. Anyways, heres the code:

//if "P"lay
		if(in.equalsIgnoreCase("P"))
		{
			//initialize ArrayList to hold computer generated numbers
			cNums = new ArrayList<Integer>();
			//initialize ArrayList to hold matching numbers
			mNums = new ArrayList<Integer>();
			//instantiate rGen
			rGen = new Random();
			
			//fill the array with random numbers 
			for(int i = 0; i < CNUMS; i++)
			{
				//fill array element with random value
				cNums.add((Integer)rGen.nextInt(MAX_VALUE));
		
				//checks number to see if already used
				for(int j = 0; j < CNUMS; j++)
				{
					//if newly generated number has been used it creates new number
					if(cNums.contains(cNums.get(i)))
					{
						cNums.remove(i);
						cNums.add((Integer)rGen.nextInt(MAX_VALUE));			
					}//end if
				}//end nested for
				
				//Show generated numbers to user
				System.out.println("Number " + (i+1) + ":\t" + cNums.get(i));
			}//end loop generating random numbers
			

The problem is that my program will still generate matching numbers inside of that loop. I've looked at it over and over again and can't figure out why it isn't doing the check if(cNums.contains(cNums.get(i)), removing the number (cNums.remove(i), then adding a newly generated number (cNums.add((Integer)rGen.nextInt(MAX_VALUE)).

Thanks in advance for any help. I'll continue to look over it and walk through it to see if I can find what I'm missing.

[Ernest Friedman-Hill]

Hi Jason,

I'm not sure I follow what you're trying to do, but wouldn't it make more sense to simply generate numbers and check if they're in the arraylist first, before adding them?

while (cNums.size() < CNUMS) {
    Integer num = rGen.nextInt(MAX_VALUE);
    if (!cNums.contains(num))
        cNums.add(num);
}

Finally, note that "contains()" is expensive when called on a long list -- it has to look at each item in the list one at a time. Sets like HashSet or TreeSet are much better at checking for a value, and they do it automatically -- a Set, by definition, can't contain duplicates. So you could simplify this further by adding random numbers to a HashSet until it was the right size, and then constructing an ArrayList to contain the result. It's just a few lines of code, but I'll leave it up to you.

[Paul Clapham]

But it IS doing that, because that's the code you wrote. I think the real question is, why doesn't that code do what you expected it to do?

Unfortunately I can't comment on that because (a) I can't tell what you expected it to do, and (b) I can't guess what it was meant to do.

First of all you have it inside a for-loop whose index is named "j", but you don't use the variable "j" anywhere inside that loop. That seems a bit odd to me.

Second, you have this code:

if(cNums.contains(cNums.get(i)))

The boolean expression there will always be true, because the cNums list will always contain an element which was selected from the cNums list.

And then if it is true (which is is) then you remove that element from the cNums list, and then you add a new element at the end of the cNums list.

So when i=0, for example, which would be the first time through the outer loop, you first add a random element to cNums. Then you repeatedly (CNUMS times) remove that random element and add a new random element. What's the point of that?

[Debashis jain]

#   for(int j = 0; j < CNUMS; j++)  
#                 {  
#                     //if newly generated number has been used it creates new number  
#                     if(cNums.contains(cNums.get(i)))  
#                     {  
#                         cNums.remove(i);  
#                         cNums.add((Integer)rGen.nextInt(MAX_VALUE));              
#                     }//end if  
#                 }//end nested for 
   The code seems a bit hazy to me....
I don't understand why u need a for loop using the "j" index to remove the 
same value from the ArrayLiist again and again.
I think no loop is required there.
2nd point is to what u are comparing the number 
"cNums.contains(cNums.get(i))"....every time it will remove the same randomly generated number List(i).
Please try to check these things it might help you. [/quote]

[Jason Sgalla]

Ernest Friedman-Hill wrote:Hi Jason,

I'm not sure I follow what you're trying to do, but wouldn't it make more sense to simply generate numbers and check if they're in the arraylist first, before adding them?

while (cNums.size() < CNUMS) {
    Integer num = rGen.nextInt(MAX_VALUE);
    if (!cNums.contains(num))
        cNums.add(num);
}

Finally, note that "contains()" is expensive when called on a long list -- it has to look at each item in the list one at a time. Sets like HashSet or TreeSet are much better at checking for a value, and they do it automatically -- a Set, by definition, can't contain duplicates. So you could simplify this further by adding random numbers to a HashSet until it was the right size, and then constructing an ArrayList to contain the result. It's just a few lines of code, but I'll leave it up to you.

I decided to go back to the drawing board with Java and start from the beginning again to re-learn everything. When I originally created this program I used Swing elements to draw a GUI but couldn't get the ScoreBoard panel to update after pressing play (everything else worked behind the scenes except for that).

The game starts by asking for 10 numbers, gets those numbers, checks for duplicates, shows them to the player, then asks for P to play Q to quit. After pressing "P" I had the code I provided that was supposed to: generate a random number, check for duplicates, then display them to the user. The program shows the number of matches to the user and which numbers actually matched up. That's it.

That is exactly what I was trying to do! I don't know anything about HashSets or TreeSets yet (although I have heard of HashSets) but I will look into those.

Why I wrote the code I wrote:
1. The for loop was to fill the arraylist with random numbers
2. The second for loop was supposed to cycle through the array and check if that newly created element was already used.
-- I see where I went wrong here. I'm not sure why I didn't see that when I was going over it, I'm still new to all this. Of coarse it is going to contain that number because it just created it
3. Remove the element if it was a duplicate
4. Create a new random number.

**NOTE
This was something I had already wrote and had working with normal arrays, but I wanted to move on try to use ArrayLists. To be honest, I'm not sure how I had this working with regular arrays (I started over from scratch) but somehow it was. Or perhaps, I was just getting lucky with my generated numbers.

Either way, thanks again for the input.

You guys/gals are great, as always.

[Paul Clapham]

This sounds familiar, because I've gone through this process many times before. Here's the process I mean:

(1) Hey, let's change this code to use this new feature!

(2) What? Why doesn't it work any more?

(3) OMG! How could I be so stupid?

So yeah. It happens to all of us. Keep up the good work!