Problem filling ArrayList

[platticus]

Hey all,

Once again I'm having a problem and am turning to CodeGuru for some direction.

I'm writing a program to simulate video poker. My class to create card objects is a huge success and I have no problems with that, as well my DeckOfCards class is now complete and working, thanks to Keang, and this post. I'm having trouble creating the PokerHand class, which creates an object that represents a 5 card poker hand of cards using card objects taken from a DeckOfCards object.

I try very hard to document my code and make it as readable as possible, so if anything is unclear in the code please let me know and I would be happy to clarify.

Here is the working code from my Card Class:

/**
* Create a simple playing card with a suit and a face value
* Suit Key: 0 --> Heart
* 1 --> Spade
* 2 --> Club
* 3 --> Diamond
* Value Key: 2-10 --> Face value
* 11 --> Jack
* 12 --> Queen
* 13 --> King
* 1 --> Ace
*/
public class Card implements Comparable
{
/*
* Declare private data members (attibutes)
*/
private int Suit;
private int Value;


/**
* Default constructor which sets the suit and value variables to 0
*/
public Card()
{
this.Suit = 0;
this.Value = 0;
}

/**
* Explicit constructor which sets the suit and value variables to those given by user
* @param s the Suit
* (Precondition: s >= 0 and s <= 4)
* @param v the Value
* (Precondition: v >= 0 and v <= 14)
*/
public Card(int s, int v)
{
this.Suit = s;
this.Value = v;
}

/**
* Get the suit of the card
* @return the Suit
*/
public int getSuit(){return this.Suit;}

/**
* Get the value of the card
* @return the Value
*/
public int getValue() {return this.Value;}

/**
* Set the suit of the card
* @param s the Suit
* (Precondition: s >= 0 and s <= 4)
*/
public void setSuit(int s) {this.Suit = s;}

/**
* Set the value of the card
* @param v the Value
* (Precondition: v >= 0 and v <= 14)
*/
public void setValue(int v) {this.Value = v;}

/**
* Compares two cards to find if they are same
*@param c the card to compar with the card that activated the method
*@return true if the cards are the same, false if not the same
*/
public Boolean equals(Card c)
{
return this.Suit == c.Suit && this.Value == c.Value;
}

/**
* Compare the face values of the cards to sort
* @param o the object to compare
* @return 0 if the equal
* @return 1 if Card 1 > Card 2
* @return -1 if Card 1 < Card 2
*/
public int compareTo(Object o)
{
//Implement cast in order to assure the correct object is being compared
if (this.Value == ((Card) o).Value)
return 0;
else if ((this.Value) > ((Card) o).Value)
return 1;
else
return -1;
}

/**
* Create a string representation of the card
* @return the String representation of the card
*/
public String toString()
{
//Declare integer value to temporarily store the suit and create a String to store the name which will be created
int suit = this.getSuit();
String SuitName;

//Declare integer value to temporarily store the value and create a String to store the name which will be created
int value = this.getValue();
String ValueName;

//Use switch statement to properly assign a Suit
switch(suit)
{
case 0: SuitName = "Hearts"; break;
case 1: SuitName = "Spades"; break;
case 2: SuitName = "Clubs"; break;
case 3: SuitName = "Diamonds"; break;
default: SuitName = "Error";
}

//Use switch statement to properly assign a Value
switch(value)
{
case 1: ValueName = "A"; break;
case 2: ValueName = "2"; break;
case 3: ValueName = "3"; break;
case 4: ValueName = "4"; break;
case 5: ValueName = "5"; break;
case 6: ValueName = "6"; break;
case 7: ValueName = "7"; break;
case 8: ValueName = "8"; break;
case 9: ValueName = "9"; break;
case 10: ValueName = "10"; break;
case 11: ValueName = "J"; break;
case 12: ValueName = "Q"; break;
case 13: ValueName = "K"; break;
default: ValueName = "Error"; break;
}

return ValueName + " of " + SuitName;
}

}



Here is the working code from my DeckOfCards Class:

/**
* Creates a deck of cards, using the card class
*/

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
public class DeckOfCards extends ArrayList
{
private ArrayList<Card> DeckOfCards = new ArrayList<Card>(52);

/*
* Construct a deck of cards with 13 cards of 4 different suits
*/
public DeckOfCards()
{

int suitCounter = 0;
int faceCounter = 1;

for (int i = 0; i < 52; i++)
{
if (suitCounter == 0)
{
DeckOfCards.add(i, new Card(suitCounter, faceCounter));

if (faceCounter < 14)
faceCounter++;
else
{
DeckOfCards.add(i, new Card(3, 11));
faceCounter = 1;
suitCounter++;
}
}
else if (suitCounter == 1)
{
DeckOfCards.add(i, new Card(suitCounter, faceCounter));

if (faceCounter < 14)
faceCounter++;
else
{
DeckOfCards.add(i, new Card(3, 12));
faceCounter = 1;
suitCounter++;
}
}
else if (suitCounter == 2)
{
DeckOfCards.add(i, new Card(suitCounter, faceCounter));

if (faceCounter < 14)
faceCounter++;
else
{
DeckOfCards.add(i, new Card(3, 13));
faceCounter = 1;
suitCounter++;
}
}
else if (suitCounter == 3)
{
DeckOfCards.add(i, new Card(suitCounter, faceCounter));

if (faceCounter < 14)
faceCounter++;
else
{
DeckOfCards.add(i, new Card(3, 13));
faceCounter = 1;
suitCounter++;
}
}
}
}

/**
* Create a string representation of the deck
* @return a string representation of the deck, numbered 1 to 52 using the card class toString method to convert the cards to string
*/
public String toString()
{
int Counter = 1;
String DeckString = "";

//Collections.shuffle((List<Card>) DeckOfCards);
Collections.sort((List<Card>) DeckOfCards);

for(int i = 0; i < 52; i++)
{
DeckString += Counter + ". " + DeckOfCards.get(i) + "\n";
Counter++;
}
return DeckString;
}

}


Here is the trouble code from my PokerHand Class:

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

public class PokerHand
{
private ArrayList<Card> PokerHand = new ArrayList<Card>(5);
private DeckOfCards PokerDeck = new DeckOfCards();

/**
* Creates a PokerHand object by extracting five cards from the deck object
*/
public PokerHand()
{
//shuffle the deck before dealing
Collections.shuffle((List<Card>) PokerDeck);

for(int i = 0; i < 5; i++)
{
Card tempCard = (Card) PokerDeck.get(i);
PokerHand.set(i, tempCard);
PokerDeck.remove(i);
}
}

/**
* Check the configuration of the PokerHand object to see if it matches any established poker hand
*/
public String checkHand()
{
String PokerHandString = "";
Collections.sort((List<Card>) PokerDeck);

return PokerHandString;
}

/**
* Draws cards from the deck object which activates the method
* @param nc the number of cards to deal
* (Precondition: nc <= hand.size())
*/
public void drawCards(int nc)
{
for(int i = 0; i < nc; i++)
{
PokerHand.add((Card) PokerDeck.get(i));
PokerDeck.remove(i);
}

}

/**
* Discards card objects from the PokerHand ArrayList object back into the deck object
* @param nd the number of cards to discard
* @param deck the deck object to discard the cards into
*/
public void discard(int nd, ArrayList deck)
{

}

/**
* Create a string representation of the deck
* @return a string representation of the poker hand, using the card class toString method to convert the cards to string
*/
public String toString()
{
String PokerHandString = "";

Collections.sort((List<Card>) PokerHand;

for(int i = 0; i < 52; i++)
{
PokerHandString += PokerHand.get(i) + "\n";
}
return PokerHandString;
}
}


The problem I'm having is in the PokerHand() constructor. When I create a tester class for the PokerHand Class and run the toString() method, I get an IndexOutOfBounds exception thrown which refers me to the line that reads : Card tempCard = (Card) PokerDeck.get(i); from my PokerHand() constructor. Apparently the ArrayList never initializes (I deduce that it is the deck) and has a size of 0.

The confusing part is that when I create and run a DeckTester Class which tests the DeckOfCards object, it works perfectly, filling a deck with 52 cards of 4 different suits.

Please not that three of my methods are not complete in their implementation in the PokerHand class. The discard(), checkHand(), and drawCards() methods are not completed since I am unable to test their implementation with the faulty constructor

Can someone please point out what my mistake in this constructor is? Thank you in advance.

[Martin O]

Apparently the ArrayList never initializes...


It does get initialized here:


...
public class PokerHand
{
private ArrayList<Card> PokerHand = new ArrayList<Card>(5);
private DeckOfCards PokerDeck = new DeckOfCards();
...



...and has a size of 0.


Which makes sense because at that point you haven't added any elements to it yet. Why did you think it should be > 0?

[Martin O]

Oh BTW, you should use Java naming conventions. For example, this:


private DeckOfCards PokerDeck = new DeckOfCards();


should be this:

private DeckOfCards pokerDeck = new DeckOfCards();


Briefly, class names get capitalized, variables & method names get camelCase, and constants (static final's) are all uppercase.

Also, FYI, this is an indispensable resource:

http://java.sun.com/docs/books/tutorial/reallybigindex.html

I learned Java, quickly, from that link.

[Martin O]

Sorry I just noticed that the object you're wondering about is a 'DeckOfCards' which you've inherited from ArrayList (I thought it was just an ArrayList). Even so, at that point, no objects have been added to the ArrayList that DeckOfCards inherited from.

First, you shouldn't have inherited from ArrayList. You should have had 'DeckOfCards' HAVE AN array list, not BE AN array list. This is an object oriented design principle--prefer encapsulation over inheritance.

Second, having a class named 'DeckOfCards' that has a member variable named 'DeckOfCards' is bad. Even if you used java naming conventions (class name would be DeckOfCards and member variable 'deckOfCards') it would still be confusing.

Wait the more I read...you used inheritance AND encapsulation? Sigh ...this is why I don't post that much, I hate getting sucked into a quagmire like this....anyway, yeah, you should pick encapsulation or inheritance, not use both at the same time. Can you explain why your DeckOfCards class both inherits from ArrayList and also has an ArrayList member variable? I don't know what you were expecting to do with that....

[nuzzle]

This is an object oriented design principle--prefer encapsulation over inheritance.


The design principle is: "prefer composition over inheritance".

[nuzzle]

My class to create card objects is a huge success and I have no problems with that, as well my DeckOfCards class is now complete and working, thanks to Keang, and this post.

Just to put things in perspective. Your code is far from the success you think it is. In fact it's quite crappy actually.

And although you felt best helped by Keang in your other thread I also put in effort only to be ignored like I didn't exist. So good luck and goodbye.

[jcaccia]

One (unfortunately very common) mistake you made was with the equals method. The signature of this method is:
public boolean equals(Object o)
If you define a method as:
public Boolean equals(Card c)
it is not overriding Object.equals and it will not be used as (I presume) you would expect by sorting methods.

A better definition of the method would be:
@Override public boolean equals(Object o) {
if (o == this) return true;
if (!(o instanceof Card)) return false;
Card c = (Card) o;
return this.Suit == c.Suit && this.Value == c.Value;
}

[platticus]

my apologies nuzzle, I did read you reply and appreciate your input.

thank you for point out my mindless override jcaccia, I completely missed that one.

Martin O, thank you very much for your constructive criticisms of my design principle. I feel like a bit of an ***, but that is part of the learning process. I took your suggestions and redesigned my class with those principles in mind, and I thank you for the time you took to point them out to me. I did my own reading on the extending/inheriting of classes (as we've not covered that in my java class as of yet), but it seems I failed miserably. thank you again.

[jcaccia]

I don't understand the logic in the constructor of the DeckOfCards class. You repeat exactly the same code in four ifs. Haven't you heard of nested loops?

[dlorde]

I did my own reading on the extending/inheriting of classes (as we've not covered that in my java class as of yet), but it seems I failed miserably. thank you again.
An important question to ask yourself before using inheritance is the "is a" question: "can I say that my subclass is a kind of the superclass?" and "can my subclass be dropped in as a direct replacement wherever the superclass is used and still have the program work?". It's clear that a DeckOfCards is not kind of ArrayList, it is a kind of 'Deck' (although actually there's no point having a Deck superclass in this situation). However, the DeckOfCards does use a List of some sort internally. If you are strict with the 'is a' question, you'll find it helps you avoid some design mistakes. It's very easy to get carried away with inheritance, and if you're not careful, it can cause a lot of misery ;)

To arrive at the simple is difficult...
R. Elisha

[nuzzle]

---

[Martin O]

The design principle is: "prefer composition over inheritance".

Oops. :blush:

[nuzzle]

An important question to ask yourself before using inheritance is the "is a" question: "can I say that my subclass is a kind of the superclass?" and "can my subclass be dropped in as a direct replacement wherever the superclass is used and still have the program work?". It's clear that a DeckOfCards is not kind of ArrayList, it is a kind of 'Deck' (although actually there's no point having a Deck superclass in this situation). However, the DeckOfCards does use a List of some sort internally. If you are strict with the 'is a' question, you'll find it helps you avoid some design mistakes. It's very easy to get carried away with inheritance, and if you're not careful, it can cause a lot of misery ;)

While DeckOfCards may not be a proper subtype of ArrayList, it definately is a proper subtype of ArrayList<Card>.

A perfectly fine design (from the pure type perspective we're discussing) could be,


public class CardList extends ArrayList<Card> {} // CardList is an ArrayList of Cards
//
public class DeckOfCards extends CardList {} // subtype of CardList
public class Hand extends CardList {} // another subtype of CardList


So generics changes things. ArrayList is unrelated to ArrayList<T> from a type perspective.

[dlorde]

While DeckOfCards may not be a proper subtype of ArrayList, it definately is a proper subtype of ArrayList<Card>.

A perfectly fine design (from the pure type perspective we're discussing) could be... <example>
Yes, your example is an elegant way to achieve the OP's goal - the CardList is an important abstraction between the ArrayList and the concrete classes.

I was trying to make a general point about inheritance - but I guess it was a mistake to use DeckOfCards to make it - festina lente... :rolleyes:

Simplicity does not precede complexity, but follows it...
A. Perlis