passing vs accessor

[gcampton]

Just wondering which would be the better option in this example. I've created a standard Card and Deck class and I'm not sure which the better option would be to either use the getter or pass the arraylist by reference.
I have opted for using the getDeck(); in populateDeck() and shuffleDeck()
here's my code:

Deck.java

import java.util.ArrayList;
import java.util.Collections;

public class Deck
{
	private ArrayList<Cards> aDeck;
	
	public Deck(ArrayList<Cards> deck)
	{
		setDeck(deck);
		populateDeck();
	}
	
	private void setDeck (ArrayList<Cards> aDeck)
	{
		this.aDeck = aDeck;
	}
	
	public ArrayList<Cards> getDeck()
	{
		return aDeck;
	}
	
	public void populateDeck()
	{
		for (Suit suit : Suit.values())
                    for (Value value : Value.values())
         	        getDeck().add(new Cards(suit,value));
	}
	
	public void shuffleDeck()
	{
		Collections.shuffle(getDeck());
	}
}

Main.java

public static void main (String[] args)
{
	ArrayList<Cards> deck1 = new ArrayList<Cards>(52);
	Deck deck = new Deck(deck1);
	deck1=deck.getDeck();
            ...

[JosAH]

Don't pass that entire ArrayList to the entire world; that makes is possible to do getDeck().clear() with possible destastrous results. Create a few other getter methods such as getCard(int rank) and getNofCards(); that way your Deck class protects the ArrayList (why not use the interface List instead?) from the world. Also, create that List/ArrayList in the constructor of the Deck.

kind regards,

Jos

[gcampton]

Don't pass that entire ArrayList to the entire world; that makes is possible to do getDeck().clear() with possible destastrous results.

In the real world yes, but is this not data hiding anyway? I thought that was the point of mutators and accessors.

Create a few other getter methods such as getCard(int rank) and getNofCards(); that way your Deck class protects the ArrayList (why not use the interface List instead?) from the world. Also, create that List/ArrayList in the constructor of the Deck.

kind regards,

Jos

1. I'm not quite sure what you have in mind for those methods and how this would get around the getDeck() method, but I can easily get the current deck count by using the size(); As my idea was as I dealt cards I would use removeRange(int fromIndex=0, int toIndex=1)...
just an idea at the moment.

2. What difference or advantage would be in using the List interface? I thought they were pretty much the same.

3. If I create in constructor,( I'm guessing you mean instead of Main).. then I only have 1 Deck, I thought it would be better this way so I could create multiple decks if I wished. And just create the arraylists in the Main or eventually in the game class. eg. 21, poker, memory classes etc...

[gcampton]

I'm not quite sure what you have in mind for those methods and how this would get around the getDeck() method

Oh, you mean instead of... get rid of getDeck()?

I guess I could put in the constructor some type of int variable, for the number of decks(lists) to create or something... I dunno how this could happen, unless it was between a set number like 1-6.

[Tolls]

For interfaces, it is better to code against the interface, not the concrete class. This allows you to change the implementation without affecting any users of your class.

As for the deck, it is a deck of cards, so presumably it can set itself up (its List<Card>) and doesn't need them provided.

And I agree that you shouldn't be returning the List anyway. Decide what you can do with your Deck and provide methods for that. Off the top of my head I can think of shuffle(), deal(int numberOfCards), nextCard(), returnCard(Card), maybe some sort of reset()? In fact, turn that into an interface, and then you can have implementations of it, like StandardDeck (52 cards), ShortDeck (there would be different types of these for different games).

The use of a Deck does not need access to the List<Card>.

[JosAH]

In the real world yes, but is this not data hiding anyway? I thought that was the point of mutators and accessors.

Indeed you're not hiding any data at all, some loony can clear that ArrayList you are exposing to the world and the results would be unpredictable. You are even exposing the concrete class (ArrayList). The methods I suggested expose a single (immutable?) card in the deck and the other returns the number of cards in the deck; all that something else should know about your deck. Abstract away more than you did above; it pays back in the end.

kind regards,

Jos

[gcampton]

Thanks guys.

[gcampton]

Also 1 more thing, what's advantage to using List?

[collin389]

Here is a way to deal a hand:

int n = //Number of cards to be delt
List<Card> handView = deck.subList(deckSize - n, deckSize);
List<Card> hand = new ArrayList<Card>(handView);

Once you shuffle the deck, you can make a sub list which automatically takes the card out of the deck. It does take it from the bottom, but that shouldn't really matter because it is shuffled anyway. Also, I have a list of Card, a separate class.

[gcampton]

Ah ok, so pretty much the same idea as i had but I was going about it a silly way.

So wouldn't a Vector also be a good idea in this?

[collin389]

Its up to you but a I don't really know how they differ besides the fact that a Vector is synchronized and an ArrayList is not.

[JosAH]

Also 1 more thing, what's advantage to using List?

Suppose you want to run that Deck class in a multithreaded application. You'd need a Vector instead of an ArrayList; also suppose that a lot of people/companies use your code already; in your scenario you're stuck; if you'd used a List in the API instead you'd be safe to use a Vector in your class while all your users don't have to change a single bit to keep their programs running.

kind regards,

Jos

[Tolls]

Re: Vector.
It's old, Java 1.1, and really ought to be viewed as defunct (bar for Java ME, apparently). Anything Vector can do, something in Collections can do just as well, if not better, including synchronisation.

As for "why List", as I said before, code to interfaces. The code using your List only cares about the functionality provided, not how that functionality is actually coded. This allows you (as Jos says) to change the how without affecting the other code at all. It means you can, if it turns out to (maybe) provide better performance, change from an ArrayList to a LinkedList...outside of Deck no one need know this.

[gcampton]

Ah ok, I thought from reading the API that Vector was pretty much the same as list, anyway. I changed Deck to an interface, but I feel I will have a lot of repeat code doing this (in classes implementing). I think perhaps abstract is the way to go, but it still has the same problem not quite as bad.

I did devise a way for a regular Deck class to take any number of decks and append them to the List, shuffle etc. which is the most reduced code and allows for any number of decks up to INT_MAX. This was before the above conversation however.

public interface Deck 
{
    // For each suit, rank create a unique card
    void populateDeck();
    // shuffles deck/s to randomise cards
    void shuffleDeck();
    // returns a single card
    List<Card> dealCard();
    int getNumOfCards();
    // returns 5
    List<Card> dealHand(); 
}

or

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

public class Deck
{
	private List<Cards> aDeck;
	private int numOfDecks;
	/**
	 * Default Constructor for Deck.
	 */
	public Deck(int numOfDecks)
	{
                if (numOfDecks<1)
                             numOfDecks=1;
		aDeck = new ArrayList<Cards>(52);
		populateDeck(aDeck);
		setNumOfDecks(numOfDecks);
		shuffleDeck();
	}
	/**
	 * Populates the Deck with 52 unique Cards.
	 */
	private void populateDeck(List<Cards> newDeck)
	{
	         for (Suit suit : Suit.values())
                         for (Value value : Value.values())
         	                newDeck.add(new Cards(suit,value));
	}
	/**
	 * Shuffles the Deck randomising the Cards order.
	 */
	public void shuffleDeck()
	{
		Collections.shuffle(aDeck);
	}
	
	public List<Card> dealCard()
	{
		return aCard; // FIXME
	}

	public List<Card> dealHand(){  }  // FIXME

	public int getNumOfCards()
	{
		return aDeck.size();
	}
	
	public void printRemainingDeck()
	{
		for (int i=0; i<aDeck.size(); i++)
			System.out.println(aDeck.get(i).toString());
	}
	/**
	 * @param numOfDecks the numOfDecks to set
	 */
	private void setNumOfDecks (int numOfDecks)
	{
		this.numOfDecks = numOfDecks;
		for (int i=0; i<getNumOfDecks(); i++)
		{
			List<Cards> aNewDeck = new ArrayList<Cards>(52);
			populateDeck(aNewDeck);
			aDeck.addAll(aNewDeck);
			aNewDeck.clear();
		}
	}
	/**
	 * @return the numOfDecks
	 */
	public int getNumOfDecks ()
	{
		return numOfDecks;
	}
}

EDIT: I think due to being so many number of deck possibilities eg. poker 1-2 (depending on game), twentyone 2-6, canaster samba 4-8, etc. I'll just do abstract, only need to change a couple methods per class.

[collin389]

Hmm, it seems you using getNumDecks and setNumDecks is wasting a lot of code. Unless you need getNumDecks for something else, you should delete both of those methods. Then change your constructor to:

public Deck(int numOfDecks)
	{
                if (numOfDecks<1)
                             numOfDecks=1;
		aDeck = new ArrayList<Cards>(52*numOfDecks);
		populateDeck(aDeck);
		shuffleDeck();
	}

and change populateDeck() to

private void populateDeck(List<Cards> newDeck)
	{
            for(int i=0; i<numOfDecks; i++)
	         for (Suit suit : Suit.values())
                         for (Value value : Value.values())
         	                newDeck.add(new Cards(suit,value));
                                     
	}

It seems like this would save a lot of code.

[gcampton]

true :) but that's not very abstract.

IMO I don't particularly see what is wrong with the 1 class, it does pretty much everything an abstraction set could achieve. And as it does everything, not allowing people to make child classes doesn't do much harm, unless in the case of wanting to add more methods.

I think pretty much the only case I can think of is wanting to add values not enum values, but integer values to the ranks, eg ace-jack 10, 10-ace corresponding value. but then in the case of other games ace-jack can have different values. It's been a little over 18 years since i've played canasta or samba but I'm pretty sure the face cards had different values. (maybe I'm thinking of points)
edit: even then this can be added to Card Class... Doesn't particularly need to be in Deck.