looking for feedback on random card generator

[Russd772]

Hello everyone,

I am just kinda looking to get some feedback on some code i wrote that generates random playing cards. the idea is that im going to try and make a text based black jack game, but this was the first step.

what im looking for is someone with some experience and know-how about Java (something i do not have, i started learning java about two weeks ago in my spare time) to tell me what the faults and downfalls of the program so far are, mostly in refrence to "will this actually work for my black jack game" im trying to prevent getting too deep into this and find out i have to start from scratch again.

any feedback would be greatly appreciated


BlackJack.java

import java.util.Random;
import java.util.Scanner;

public class BlackJack {

	static int handsize;
	public static void main(String[] args) {
		
		Random randomCard = new Random();
		DeckOfCards getCard = new DeckOfCards();
		// set number of cards i want returned, or printed out 
		handsize = 6;
		// not sure why i named this loop maybe for future refrence 
		outerLoop:
			// run loop for each card i want
			for(int i = 0; i < handsize; i++){
				getCard.Deal(1 + randomCard.nextInt(52), handsize);
				if(getCard.isUsed() == true){
					//if card is used decrement i so i still get full handSize 
					i--;
				}	
	}
	
	}
	
}

DeckOfCards.java

// use Random util from Java's preloaded library 
import java.util.Random;

public class DeckOfCards {
	// set saveCard to 0 
	public static int saveCard = 0;


// declare an int array to use for storing values already generated 
	public static int check[] =new int[53];                                  
  // declare variables for importing values 
 static int handSize;
 static int newHand;
	 // return the initial size of the hand 
	 public static int getNewHand() {
	return newHand;
}
	 // set the initial size of the hand 
public static void setNewHand(int newHand) {
	DeckOfCards.newHand = newHand;
}
   // set the size of new hand for use with getting more cards 
	public void  setHandSize(int handSize){
			this.handSize =handSize;
		}
	// return handsize 
	 public static int getHandSize(){
		 return handSize;
	 }
	
static boolean used;

		// Give the cards a "face"  
	   // the initial value of "NULL" is because using a value at cardface[0] causes 
	   // problems later on and i will point them out.

		
	    public boolean isUsed() {
	return used;
}
public void setUsed(boolean used) {
	this.used = used;
}

		public static String[] cardFace = {"Null","Ace of Hearts", "Two Of Hearts", "Three Of Hearts", "Four Of Hearts",
			"Five Of Hearts", "Six Of Hearts", "Seven Of Hearts", "Eight Of Hearts", "Nine Of Hearts", 
			"Ten Of Hearts", "Jack Of Hearts", "Queen Of Hearts", "King Of Hearts",
			                          "Ace of Spades", "Two Of Spades", "Three Of Spades", "Four Of Spades",
			"Five Of Spades", "Six Of Spades", "Seven Of Spades", "Eight Of Spades", "Nine Of Spades", 
			"Ten Of Spades", "Jack Of Spades", "Queen Of Spades", "King Of Spades",
									   "Ace of Diamonds", "Two Of Diamonds", "Three Of Diamonds", "Four Of Diamonds",
			"Five Of Diamonds", "Six Of Diamonds", "Seven Of Diamonds", "Eight Of Diamonds", "Nine Of Diamonds", 
			"Ten Of Diamonds", "Jack Of Diamonds", "Queen Of Diamonds", "King Of Diamonds",
									   "Ace of Clubs", "Two Of Clubs", "Three Of Clubs", "Four Of Clubs",
			"Five Of Clubs", "Six Of Clubs", "Seven Of Clubs", "Eight Of Clubs", "Nine Of Clubs", 
			"Ten Of Clubs", "Jack Of Clubs", "Queen Of Clubs", "King Of Clubs"};
	
	
	
    
	    // "shuffle the deck"
		static Random shuffleDeck = new Random();
		// set value of c so it can be used in both loops 
		

		
		// return the total cards used in the game 
	public static int totalCards(){
		return getHandSize() + getNewHand();
	}
	  
	// start method to deal cards 
    public static void Deal(int cardNum, int handSize){
    	 
    	
    	
    
		                                                     
			    // start  loop to check if value is already used 
			    innerloop:                                                         
			    for(int i = 0; i <= handSize; i++){   
			    	// this is where the error from cardFace[0] comes from.  Since the value of 
			    	// the array is defaulted to 0 then this will cause an infinite loop. 
				if(check[i] == cardNum) {
				// set used to true if card has been used 
					used = true;
					 // break  loop if card is used since no need to continue  
					 break innerloop;
					 
					}
			// check to make sure all possible array values are checked 
				else if(i == handSize  && check[i] != cardNum){
				// assign used value to array so we can check it next loop 
					check[saveCard] = cardNum;
					String getCard = cardFace[cardNum];
					// output card's "face value"
					System.out.println(getCard);
					saveCard++;
				// commented out, used for troubleshooting code issues 	
				 //	System.out.println("Card: " + randomCard + " Value in check: " + check[i] +" i: " + i + " value of c: " + c );
					
					// break innerloop, this is here just for good measure so we make sure no
					// duplicate cards are printed out.
					break innerloop;
				}
				
				
					    
					                       
				}
				
		}	
			
		
		}

Thanks in advance
Russ

[Russd772]

well i thought this was complete but there are errors, guess im back to debug mode lol. i keep getting an index out of bounds error randomly.. sometimes it works and others i get this error, i think it must have something to do with the way im storing the values in check[], something keeps making it loop untill the array is full and it throws the error...

edit: just to copy/paste error
Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException: 53
at DeckOfCards.Deal(DeckOfCards.java:94)
at BlackJack.main(BlackJack.java:17)

[kaydell2]

To make the exception happen so that you can repeat it every time, you can pass a constant to the constructor for Random.

Random randomCard = new Random(seed);

You could have a for-loop, trying various values for seed, until you find one that causes the exception. Then you could eliminate the for-loop and just pass the seed value into the constructor to cause the exception every time.

The stack trace tells you that the exception happened in the Deal() method on line 94, so look there and look at the index that you are using to access an element of the array there.

[kaydell2]

You asked for feedback.

As far as the classes and objects, I like the fact that you have a class for DeckOfCards and a BlackJack class that seems like a good idea. i would also have the following classes:

Card
Hand

I would also have the DeckOfCards class have an ArrayList of Card objects. Rather than dealing cards at random and seeing if they're already used, I would put all the possible cards into the ArrayList of cards and shuffle them with a call to:

Collections.shuffle(cards);

Then the DeckOfCards class would deal the cards one-at-a-time from the ArrayList of cards.

Post again on this thread is you want more help.

[Russd772]

I resolved this on my own already sorry if anyone took time to respond.

I'm trying to use the list to do this but everytime i cal the method the list gets shuffled so a card that was at index(1) could end up at index(2) next time i call the method, which would result in a repeat card.
i tried to put the collections.shuffle in a seperate method so i could shuffle when i wanted to but it will not accept the arraylist as a valid list to shuffle.

here is the code i have for the Cards.java class

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



public class Cards {

	public static void getCard(int card){
		ArrayList <String> cardFace =  new ArrayList<String>(
	
			Arrays.asList("Null","Ace of Hearts", "Two Of Hearts", "Three Of Hearts", "Four Of Hearts",
		"Five Of Hearts", "Six Of Hearts", "Seven Of Hearts", "Eight Of Hearts", "Nine Of Hearts", 
		"Ten Of Hearts", "Jack Of Hearts", "Queen Of Hearts", "King Of Hearts",
		                          "Ace of Spades", "Two Of Spades", "Three Of Spades", "Four Of Spades",
		"Five Of Spades", "Six Of Spades", "Seven Of Spades", "Eight Of Spades", "Nine Of Spades", 
		"Ten Of Spades", "Jack Of Spades", "Queen Of Spades", "King Of Spades",
								   "Ace of Diamonds", "Two Of Diamonds", "Three Of Diamonds", "Four Of Diamonds",
		"Five Of Diamonds", "Six Of Diamonds", "Seven Of Diamonds", "Eight Of Diamonds", "Nine Of Diamonds", 
		"Ten Of Diamonds", "Jack Of Diamonds", "Queen Of Diamonds", "King Of Diamonds",
								   "Ace of Clubs", "Two Of Clubs", "Three Of Clubs", "Four Of Clubs",
		"Five Of Clubs", "Six Of Clubs", "Seven Of Clubs", "Eight Of Clubs", "Nine Of Clubs", 
		"Ten Of Clubs", "Jack Of Clubs", "Queen Of Clubs", "King Of Clubs"));
	 
	System.out.println(cardFace.get(card));
	}
	public static void shuffleDeck(){
		Collections.shuffle(cardFace); 
}
}

[kaydell2]

I see what you're trying to do. If you modify your code slightly to declare the cards as an instance variable, you can have the ArrayList of Card objects available in other methods.

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

public class Deck {
	
	// declare the ArrayList here so that it can be used elsewhere in the class
	ArrayList <String> cards;

	public Deck(){
		
		// initialize the ArrayList here in the constructor
		cards =  new ArrayList<String>(
				Arrays.asList("Null","Ace of Hearts", "Two Of Hearts", "Three Of Hearts", "Four Of Hearts",
						"Five Of Hearts", "Six Of Hearts", "Seven Of Hearts", "Eight Of Hearts", "Nine Of Hearts", 
						"Ten Of Hearts", "Jack Of Hearts", "Queen Of Hearts", "King Of Hearts",
						"Ace of Spades", "Two Of Spades", "Three Of Spades", "Four Of Spades",
						"Five Of Spades", "Six Of Spades", "Seven Of Spades", "Eight Of Spades", "Nine Of Spades", 
						"Ten Of Spades", "Jack Of Spades", "Queen Of Spades", "King Of Spades",
						"Ace of Diamonds", "Two Of Diamonds", "Three Of Diamonds", "Four Of Diamonds",
						"Five Of Diamonds", "Six Of Diamonds", "Seven Of Diamonds", "Eight Of Diamonds", "Nine Of Diamonds", 
						"Ten Of Diamonds", "Jack Of Diamonds", "Queen Of Diamonds", "King Of Diamonds",
						"Ace of Clubs", "Two Of Clubs", "Three Of Clubs", "Four Of Clubs",
						"Five Of Clubs", "Six Of Clubs", "Seven Of Clubs", "Eight Of Clubs", "Nine Of Clubs", 
						"Ten Of Clubs", "Jack Of Clubs", "Queen Of Clubs", "King Of Clubs"));
		// You might as well shuffle the cards here in the constructor
		Collections.shuffle(cards); 
	}
	
	// you can deal a card here.  Remove cards from the ArrayList as they are dealt
	public String dealCard() {
		// TODO
		return "";
	}
	
}

I would rather have an ArrayList of Card objects than String objects, but I think that this would be a step in the right direction.

[Russd772]

yeah im going to make each card an object because that way i can give each card a value and a name , something like this

public class Cards {
	String name;
	short value;
	
	public void AceHearts(){
		this.name = "Ace Of Hearts";
		this.value = 11;
		}

}

im not too sure if im doing this right but i will get it. my original post was because i thought i had a finished product but i was wrong, thanks to your help i can see where my original train of thought was flawed.

again your help is greatly appreciated

Thanks
Russ

[DarrylBurke]

Russ, have you studied enums yet?

db

[Russd772]

i have seen them when looking around but not really looked into them yet. im still kind of struggling with basic concepts so im trying to get some of those down before i move on to new material.