Help with ArrayLists, NoSuchElementException

[Justin Coombs]

Howdy

I have been trying to make a little hobby card game program using ArrayLists to hold players' hands and I seem to be stuck on the most basic thing. I am trying to pass instantiated Card objects from the "ArrayList<Card>deck" in class "Card" over to the Player classes instance variable "hand". I am just a beginner with ArrayLists, and the syntax for iterators is still confusing to me. I'm not even sure if my method to display the hand array with the iterator is even right, or if I even need the iterator(i.e could i just write hand.toString();...i get an error either way)

Here's the code:

import java.util.ArrayList;
import java.util.Iterator;


public class Player implements Iterator {

	private int cardsInHand;
	private int pointsThisRound;
	private int pointsThisGame;
	private int pointsThisCareer;
	private int chips;
	private String name;
	private ArrayList<Card> hand = new ArrayList<Card>();
	
	//Calls to card class
	public void receiveHand(){
		Card temp = new Card();
		this.hand = temp.getHand(2);

	}
	//I don't even know if this is how i go about displaying an arraylist
	public void displayArr(){
		Iterator <Card> it = hand.iterator();
		System.out.println(it.toString());
	}
}

import java.util.ArrayList;
import java.util.Iterator;


public class Card implements Iterator{
	
	private String suit;
	private String face;
	private int logicFace; //for math-based "game engine" specific to game class
        private int logicSuit; //arbitrary
	private boolean isInHand;
	private boolean isInDeck;
	
	//Array used to initialize and shuffle for simplicity; turned to ArrayList before game begins
	private Card[] preDeck = new Card[52];

	private ArrayList <Card> deck = new ArrayList <Card>();
	private String[] suitArr = new String[]{"Hearts", "Spades", "Diamonds", "Clubs"};
	private String[] faceArr = new String[]{"2", "3", "4", "5", "6", "7", "8", "9", "10", "J", "Q", "K", "A"};
	private int[] logicFaceArr = new int[]{2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14};
	private int[] logicSuitArr = new int[]{1, 2, 3, 4};
	
	public void initDeck(){
	int a = 0;
	for(int i = 0; i < 4; i++){
		for(int j = 0; j < 13; j++){
			preDeck[a] = new Card();
			preDeck[a].suit = suitArr[i];
			preDeck[a].face = faceArr[j];
			preDeck[a].logicFace = logicFaceArr[j];
			preDeck[a].logicSuit = logicSuitArr[i];
			preDeck[a].isInDeck = true;
			preDeck[a].isInHand = false;
			deck.add(preDeck[a]);
			a++;
		}

	}

	}

	public void displayPreDeck(){
		for(int i = 0; i < preDeck.length; i++){
			System.out.println(preDeck[i].face + " of " + preDeck[i].suit + "\tLogicFace = " + preDeck[i].logicFace + "\tLogicSuit = " + preDeck[i].logicSuit);
		}
	}
	public ArrayList <Card> getHand(int numCards){
		ArrayList <Card> temp = new ArrayList <Card>();
		int x = 0;
		while(x < numCards){
		temp.add(this.deck.iterator().next());
		}
			return temp;
		
	}
}

public class GameManager {

	public static void main(String[] args){
		
		Card c = new Card();
		Player p1 = new Player();
		c.initDeck();
		c.displayPreDeck();
		p1.receiveHand();
		p1.displayArr();
	}
}

Output

2 of Hearts	LogicFace = 2	LogicSuit = 1
3 of Hearts	LogicFace = 3	LogicSuit = 1
4 of Hearts	LogicFace = 4	LogicSuit = 1
5 of Hearts	LogicFace = 5	LogicSuit = 1
6 of Hearts	LogicFace = 6	LogicSuit = 1
7 of Hearts	LogicFace = 7	LogicSuit = 1
8 of Hearts	LogicFace = 8	LogicSuit = 1
9 of Hearts	LogicFace = 9	LogicSuit = 1
10 of Hearts	LogicFace = 10	LogicSuit = 1
J of Hearts	LogicFace = 11	LogicSuit = 1
Q of Hearts	LogicFace = 12	LogicSuit = 1
K of Hearts	LogicFace = 13	LogicSuit = 1
A of Hearts	LogicFace = 14	LogicSuit = 1
2 of Spades	LogicFace = 2	LogicSuit = 2
3 of Spades	LogicFace = 3	LogicSuit = 2
4 of Spades	LogicFace = 4	LogicSuit = 2
5 of Spades	LogicFace = 5	LogicSuit = 2
6 of Spades	LogicFace = 6	LogicSuit = 2
7 of Spades	LogicFace = 7	LogicSuit = 2
8 of Spades	LogicFace = 8	LogicSuit = 2
9 of Spades	LogicFace = 9	LogicSuit = 2
10 of Spades	LogicFace = 10	LogicSuit = 2
J of Spades	LogicFace = 11	LogicSuit = 2
Q of Spades	LogicFace = 12	LogicSuit = 2
K of Spades	LogicFace = 13	LogicSuit = 2
A of Spades	LogicFace = 14	LogicSuit = 2
2 of Diamonds	LogicFace = 2	LogicSuit = 3
3 of Diamonds	LogicFace = 3	LogicSuit = 3
4 of Diamonds	LogicFace = 4	LogicSuit = 3
5 of Diamonds	LogicFace = 5	LogicSuit = 3
6 of Diamonds	LogicFace = 6	LogicSuit = 3
7 of Diamonds	LogicFace = 7	LogicSuit = 3
8 of Diamonds	LogicFace = 8	LogicSuit = 3
9 of Diamonds	LogicFace = 9	LogicSuit = 3
10 of Diamonds	LogicFace = 10	LogicSuit = 3
J of Diamonds	LogicFace = 11	LogicSuit = 3
Q of Diamonds	LogicFace = 12	LogicSuit = 3
K of Diamonds	LogicFace = 13	LogicSuit = 3
A of Diamonds	LogicFace = 14	LogicSuit = 3
2 of Clubs	LogicFace = 2	LogicSuit = 4
3 of Clubs	LogicFace = 3	LogicSuit = 4
4 of Clubs	LogicFace = 4	LogicSuit = 4
5 of Clubs	LogicFace = 5	LogicSuit = 4
6 of Clubs	LogicFace = 6	LogicSuit = 4
7 of Clubs	LogicFace = 7	LogicSuit = 4
8 of Clubs	LogicFace = 8	LogicSuit = 4
9 of Clubs	LogicFace = 9	LogicSuit = 4
10 of Clubs	LogicFace = 10	LogicSuit = 4
J of Clubs	LogicFace = 11	LogicSuit = 4
Q of Clubs	LogicFace = 12	LogicSuit = 4
K of Clubs	LogicFace = 13	LogicSuit = 4
A of Clubs	LogicFace = 14	LogicSuit = 4
Exception in thread "main" java.util.NoSuchElementException
	at java.util.AbstractList$Itr.next(Unknown Source)
	at Card.getHand(Card.java:58)
	at Player.receiveHand(Player.java:18)
	at GameManager.main(GameManager.java:10)

[Steve Luke]

Your error message is caused because the Player is creating a new Card, then trying to get a hand from it. But the new Card has never been initialized, and so is empty. When you try to get the next card, you never check that there is a next card (there won't be) and so you get the error message.

Your entire design is flawed, though, which is part of the reason you have this problem. Your Card class represents a card - it has a suit and a face. But it also holds the deck... does that make sense? When you are playing poker, does each card have a deck? Also think about how you are using the Card instance: The Player first makes a blank card, one with no face or suit. The Player then gets a hand from this blank card. First: players shouldn't be able to make cards, they should be given or have to take them. Second, a blank card should not be allowed to exist, it should require a suit and face - have you ever seen a blank card used in a card game? Finally, a card doesn't generate a hand, the hand is made by cards received from the deck.

The deck should be separate from the Card. You should have one deck - not one per Card, and the deck should not be managed or accessed by the Card, rather the GameManager should probably manage and access the deck - giving the cards to the Player or letting the Player ask for the Cards.

You also need to learn a bit about Collections and how they work. Take a peak at this: http://docs.oracle.com/javase/tutorial/collections/interfaces/collection.html That has a section for traversing a collection (like a List) that should help. Making all your Classes implement Iterator is un-necessary and misleading - don't do that.

[Justin Coombs]

Keep in mind I'm a beginner and I just want to figure out how to make things work without errors right now

The Player has to create a new Card to access the methods of the Card class...I thought that that is a normal thing; to instantiate an instance of a separate class in order to access their methods. Why would I need to initialize all of its fields (with a constructor I haven't made yet :P) when its just a local variable made to access methods in the card class and then disappear after Player.hand has been assigned the return data from Card.getHand. The name temp is named temp because its only there to do a job temporarily and then go away.

So if I made a separate deck class as you suggest, I would still be required to essentially create the exact same method in the deck class in order to getCard 52 times from the card class...this will bring me right back to the same issue I'm having right now, which is how do I pass an ArrayList. It would just be the Player class calling the deck instead of the card in that case...either way, I still don't know how to do it.

[Steve Luke]

Justin Coombs wrote:Keep in mind I'm a beginner and I just want to figure out how to make things work without errors right now

How to get this to work involves more than one change, mostly because of your design. I told you how to fix the immediate problem (initialize the deck using the card the player creates), but that will lead to others. I better design is the correct answer because it isn't simply reacting to errors, it is a plan being executed - planing and executing may take more time to get started, but usually take less time (and less pain and suffering) to complete.

The Player has to create a new Card to access the methods of the Card class...I thought that that is a normal thing;

You are thinking too much about the current implementation - which is flawed. The Player needs a hand. The hand should come from a deck. It isn't about creating the card, it is about creating a hand.

to instantiate an instance of a separate class in order to access their methods. Why would I need to initialize all of its fields (with a constructor I haven't made yet :P) when its just a local variable made to access methods in the card class and then disappear after Player.hand

The fact that you have a class whose instances are used for two different and unrelated purposes: 1 where you need the suit and face initialized, and 1 where you don't (but you do need the deck initialized) is a good indicator that you actually have two different Classes. One (the Card) that has a suit and a face, and one that is a deck (collection of Cards). Trying to combine the two leads to confusion and problems - like the confusion and problem you have right now.

has been assigned the return data from Card.getHand. The name temp is named temp because its only there to do a job temporarily and then go away.

So if I made a separate deck class as you suggest, I would still be required to essentially create the exact same method in the deck class in order to getCard 52 times from the card class...this will bring me right back to the same issue I'm having right now, which is how do I pass an ArrayList. It would just be the Player class calling the deck instead of the card in that case...either way, I still don't know how to do it.

I am not saying there has to be a separate Deck class, but that the deck (the collection of Cards) should not be handled by the Card. The fact that you are thinking about simply substituting the Deck for Card indicates you are not thinking it through far enough.

Let's say you go ahead and make the Deck class. Where and when do you get it and use it? Does each player get its own deck? That usually isn't the case, it usually is the case that all Players share the same Deck. So the deck should have a greater scope than a player - so the player should no make the deck.

What about the next time the player is supposed to draw cards? Does the player get to choose from a fresh set of 52 cards, or should s/he be limited to using the cards that aren't already played? It is usually the latter - in which case the deck should not be something transiently created in a temp variable, but should last through the entire game.

So this Deck should probably not be created by the Player, because multiple players should be pulling from the same Deck, and the Deck should not be created for each time new cards are drawn because the new cards should come from the pool of cards left over from previous draws. Where would be the right place to create and store an instance of this Deck? You only have one per game, and it lasts for the length of the game? That doesn't sound like Card to me (you expect 52 of those) and it doesn't sound like Player (you will have multiple.) It also doesn't sound like a method-local variable. How about creating it and storing it in the GameManager? After all, managing the Deck is something that seems natural to be handled by who ever is responsible for managing the game.

If you decide that there should not be a Deck class, that the deck should still just be an ArrayList<Card> then that is fine - it has little effect on anything I talked about. It still should not be created by the Card (because there are not 52 decks) or the Player (because there are not multiple decks) and should be in a scope where it can be referenced multiple times without having to be re-created.

...issue I'm having right now, which is how do I pass an ArrayList...either way, I still don't know how to do it.

If the question is simply how do you pass an ArrayList, well, how do you pass anything in Java? Hint: you are already passing something to the getHand() method. My point is, that won't help you.

[Justin Coombs]

so the player should no make the deck.

Am I missing something here? The player does not make the deck. I don't understand where you're making that assumption. Look at the code for class Player I posted. It has 2 methods, one to display an ArrayList and the other to simply receive 2 cards from the Card class. The player does not have anything to do with instantiating or initializing the fields of a deck, or the Card objects contained within that deck. deck is called in class Card, but class Player has nothing to do with making a deck. In fact, if you look at game manager and the order in which the stack would execute, you would see that by the time the execution gets to touching class Player, the preDeck had already been created, filled with Card objects and all necessary fields stamped on each card, and ArrayList deck had already had 52 card objects appended to it. I don't see how player has anything to do with making a deck.

Why is it bad style to basically say "A Card has such and such instance variables, and when we take 2 or more cards and add them to an ArrayList of type Card, we shall call it something different to indicate its nature of plurality, such as - in this case - Card(Singular) vs deck(2 or more Cards). I don't understand how that is bad design style. Also, local variables are consumables. They can be just arbitrary things you don't give a hoot about; you only use it as a means to an end. And when that local variable goes out of scope, its all fine and well because I never intended to keep the thing around anyway...so why should I bother to initialize its fields in the case of the Player.receiveHand? If its only a tool card to simply allow access to class card and I don't want to keep it afterwards, why make it a King of spades for example? It's just gonna disappear when I execution passes the closing brace anyway. Not only that, but also, why in the heck would I want to make ANOTHER King of Spades when there's already one in Card.deck. I was always taught to think about programming design in terms of tangible objects. Like "what does a Card object know about itself(i.e. face, suit) and what does and object do(its methods)." In keeping with that approach to object oriented design, I don't understand why a deck in class card is such a bad idea. All a deck is is a collection of cards. All an ArrayList, in this program, is a collection of objects. The getHand method essentially just takes Cards from the bigger pile dubbed "deck" and arranges them into a smaller pile dubbed "temp" which can then be passed to the player to become his own hand, quite like in real life. The only way to make it more proper i guess would be to make a dealer class that handles the passing of cards...but then the dealer class would still have to create a card in order to access the card objects of the card class, and as you said, a [dealer/player] in real life does not simply snap their fingers and CREATE a card(especially not a BLANK one), so again, back to the same issue.

I understand that there are certain standards in object oriented design that must be adhered to for code to be readable and maintainable and things like that, as well as make sense to people adding to or patching previously written code, but you're not going to get java whizzes in the "beginning java" section of the forums. As a newbie, its extremely frustrating trying to learn and understand how to use the colossal library of tools available in the java API without getting error messages, and its difficult to get so much advise about proper design patterns and things like that when you try to reach out for help about a specific issue that personally is what you are stuck on at the moment. I'm not stuck on proper class layout and software design principles. I haven't even got there yet. I'm still trying to figure out how to make the machine count to 10.

[Steve Luke]

Justin Coombs wrote:

so the player should no make the deck.

Am I missing something here? The player does not make the deck. I don't understand where you're making that assumption. Look at the code for class Player I posted. It has 2 methods, one to display an ArrayList and the other to simply receive 2 cards from the Card class. The player does not have anything to do with instantiating or initializing the fields of a deck, or the Card objects contained within that deck. deck is called in class Card, but class Player has nothing to do with making a deck. In fact, if you look at game manager and the order in which the stack would execute, you would see that by the time the execution gets to touching class Player, the preDeck had already been created, filled with Card objects and all necessary fields stamped on each card, and ArrayList deck had already had 52 card objects appended to it.

That is where you are wrong. The manager makes a Card, and initializes its deck. The Player makes a new Card. The new card has its own deck - therefore by making a new Card the player is making the new deck. The player never initializes the new deck, which is part of the new Card. So the new deck is empty. So the player tries to get the cards from the new deck (inside the new Card) you get the NoSuchElementException.

The Card that the player makes (named temp) is not the same Card that the manager makes (named c). It is a different instance, and the new instance is never initialized with the cards. I told you this in my first reply.

The rest has been why your design led to this mistake (and others).

I don't see how player has anything to do with making a deck.

I hope you do now. Let me know if you don't because it is the fundamental problem that both causes the error and my concerns about the design.

Why is it bad style to basically say "A Card has such and such instance variables, and when we take 2 or more cards and add them to an ArrayList of type Card, we shall call it something different to indicate its nature of plurality, such as - in this case - Card(Singular) vs deck(2 or more Cards). I don't understand how that is bad design style.

A class should do one thing, and do it well. It makes it a lot more modular, easy to code, and easy to troubleshoot. Your Card is doing two different things. It represents both a playing card, and a collection of playing cards. No one single instance of the Card class is meant to do both - you mean to have the Card you make in the main method be used to initialize the collection of cards, and you mean the cards in the player's hands represent playing cards. The inter-mixing of these two separate and unrelated tasks makes the Card class more complicated than it should be, and makes using the class harder than it should be.

Also, local variables are consumables. They can be just arbitrary things you don't give a hoot about; you only use it as a means to an end. And when that local variable goes out of scope, its all fine and well because I never intended to keep the thing around anyway...so why should I bother to initialize its fields in the case of the Player.receiveHand?

The deck is part of the Card instance. If you don't initialize the deck in the Card than you do not have cards in the deck, and you can't get cards from the deck. Your tool is vapor, useless. In order to make it do something it needs the substance to work with. In your scenario, the substance is the deck, and the deck is empty.

If its only a tool card to simply allow access to class card and I don't want to keep it afterwards, why make it a King of spades for example?

First: again, this a sign of bad design. I am not trying to rag on you, I am trying to explain the concept because I know you are a beginner and this stuff is hard to grasp. But once you do it makes the rest of your coding career easier.

So back to the problem: You have a Card which does not act as a Card. That is the first tip that you have things wrong. A Card should always act like a Card. The fundamental definition of a playing card is its face and suit. If you are making a Card where that doesn't matter, then that should trigger something in your brain that says 'aha! this is not the way to do this, let me try a different approach.'

It's just gonna disappear when I execution passes the closing brace anyway.

It isn't really about the life of the Object. That doesn't matter. What matters is how you use the Class. Give a Class too many divergent tasks and it becomes hard to use. You wrote the Card class, gave it two distinctly different things to do, and now you are having a hard time using it.

Not only that, but also, why in the heck would I want to make ANOTHER King of Spades when there's already one in Card.deck.

Oh, and therein lies the major design flaw. Since every Card has its own deck, if you were to properly initialize each Card such that you could actually use it, each Card would have it's own deck, and each deck would have its own King of Spades. You won't have one King of Spades, you would have 52.

Okay, you say, I just won't initialize the deck in the Cards that I will be playing with. I will only initialize the Cards I need to create hands and get new cards from - these transient Cards in the method that go away as soon as I am done with them. But still, each Card you use for this case has its own deck, and in order to get a hand from the Card's deck, you need to initialize it. That means every receiveHand method has its own new Card instance, each of the getHand's Card instances has its own deck (which must be initialized to function) and so each receiveHand method has its own King of Spades. That means at least one King of Spades for each Player, but probably more once you get to drawing new cards. Pull the deck out of the Card class, put it into the control of the GameManager, and this problem goes away.

I was always taught to think about programming design in terms of tangible objects. Like "what does a Card object know about itself(i.e. face, suit) and what does and object do(its methods)." In keeping with that approach to object oriented design, I don't understand why a deck in class card is such a bad idea.

Does each card in a pack of playing cards have a deck? When you try to get a hand of cards, do you pull out the blank no-face un-suited card and take your hand from it? No. The deck is a separate entity, a thing which contains Cards. The Card is a separate entity, with no connection to any other card. It can be moved about and handled independently. Your class describes a situation where each Card has a collection of Cards, and you use special throw-away Cards to access a deck of Cards. This special temporary card is responsible for the other cards. This card does not know about its face or suit, since that stuff is not important. It does know about the rest of the cards, and their order, and which ones should be dealt next. Does that sound like it models real life?

All a deck is is a collection of cards.

Right. A deck is a collection of Cards. A deck is not a property of a Card, like the face and suit are. It is a separate container.

All an ArrayList, in this program, is a collection of objects. The getHand method essentially just takes Cards from the bigger pile dubbed "deck" and arranges them into a smaller pile dubbed "temp" which can then be passed to the player to become his own hand, quite like in real life.

That makes sense, but look at what is doing the action, and what the big pile of cards is. Who is doing the action: a Card. A Card takes Cards from a pile and gives them to the Player. What is the big pile of cards? A property of the Card. This is not like the real world at all. A player does not go to a Card to get a hand, he either goes to a dealer or a stack of cards on the table (a deck).

The only way to make it more proper i guess would be to make a dealer class that handles the passing of cards...

Right - but really you already have something that could act like a dealer (even if not named as such) - and that is the GameManager.

but then the dealer class would still have to create a card in order to access the card objects of the card class, and as you said, a [dealer/player] in real life does not simply snap their fingers and CREATE a card(especially not a BLANK one), so again, back to the same issue.

Right - exactly. The biggest problem is the link between the Card and the Deck - and mainly because that connection is in the wrong direction, and is managed by the wrong class.

I understand that there are certain standards in object oriented design that must be adhered to for code to be readable and maintainable and things like that, as well as make sense to people adding to or patching previously written code, but you're not going to get java whizzes in the "beginning java" section of the forums.

No, that isn't my intent. My intent is to try to get you thinking about the design and how it affects the flow of data through your application. You should learn to think about that, and when you do that it makes your life easier. Passing it off as 'I am new here' is a cop-out, because you can come up with a good design when you think about it. I am not talking 'patterns' or anything.

As a newbie, its extremely frustrating trying to learn and understand how to use the colossal library of tools available in the java API without getting error messages, and its difficult to get so much advise about proper design patterns and things like that when you try to reach out for help about a specific issue that personally is what you are stuck on at the moment.

Sorry about the frustration. I know it can be intimidating. What I see in your code indicates that fixing the immediate problem (which I told you how to do already) will just lead to more problems. Many more. Whereas sitting back, thinking about the issues I brought up, and thinking about data flow, and planning your application rather than reacting to error messages, will make it easier.

I'm not stuck on proper class layout and software design principles. I haven't even got there yet.

I disagree. Your main problem here is class layout. You should think bout class layout before you ever touch a keyboard.

I'm still trying to figure out how to make the machine count to 10.

Not quite. You are stuck using a class you wrote because you made it do too many things, and don't quite understand how it works. The problem is not that you have the NoSuchElementException - I told you the solution to that (first paragraph of my first response). The problem is that you don't under stand why you should have to do that (as per your questions in the quoted post). And that really comes from the fact that the Card doesn't really act the way you think it does - that you don't have a single deck, like you want, you have a deck per Card instance. So even when you get past the exception by initializing the deck in the receiveHand method you will get unexpected behavior. And that is all about class design.