1. Post #1

    April 2012
    10 Posts
    Hey,
    I have written this very simple UNO game and feel like i NEED some criticism.
    I couldn't figure out how I could do the permissions for the player not to directly control the cards in their hand. So i made them a separate class (I have left out validation arguments from the Hand's Class as i really don't know what I should use!)
    Also because of the way I have done it the Player has a reference to their hand but the hand doesn't have a reference to which player it belongs to!
    Also the hand validation happens in the Hand class, i didn't know if it should be a separate class, also how would I extend the Card class to include the special cards such as pick up two or change suit?
    Any and all criticism's are very welcome, I would like to see how I could improve my Java, Thanks!

    My program consists of 3 main parts
    - a pretty generic card and deck class
    - a player and hand class (player's logic in the player class, all he does is select the card they wish to play .. hand validation and actions carried out in hand)
    - a game class from which the running of the game is preformed from

    First the Game Class itself:

    Code:
    public class Game{
    	int numPlayers=2;
    	int startNumber=7;
    	//players and hands seperate so players can't change their cards
    	Player player[] = new Player[numPlayers];
    	Hand hands[] = new Hand[numPlayers];
    	Card topCard;
    	Deck deck;
    
    	public Game(){
    		deck = new Deck();
    		for (int i=0; i<numPlayers; i++){
    			//naming of players and hands should be the same, I havn't figured the best way to do this yet
    			hands[i] = new Hand(deck, (String)(""+(i+1)), startNumber);
    			player[i] = new Player(hands[i], (String)(""+(i+1)));
    		}
    	}
    	private void run(){
    		Boolean inGame=true; //inGame is a condition which could be used
    		topCard=deck.drawCard(); //for first run draw a new top card.
    		while(inGame){ //runs untill System.exit is called
    			for (int i=0; i<numPlayers; i++){
    				player[i].turn(topCard); //preform the players logic which will set the card they wish to play
    				Card temp = topCard;
    				topCard=hands[i].play(topCard); //sets the new top card to 
    				if (temp!=topCard){ //checks if player has returned a new card or can't go
    					deck.recieveCard(temp); //if player has played a card return the card underneath to the deck
    				}
    				System.out.println("Top Card: "+topCard+"\n\n");
    				if (hands[i].size()==0){ //checks if a player has no more cards and therefore has won
    					System.out.println("----------\n"+player[i]+" WON!");
    					System.exit(i);
    				}
    			}
    		}
    	}
    	public static void main(String[] args) {
    		Game uno = new Game();
    		uno.run();
    	}
    }
    Now the Hand and Player Classes:

    Code:
    import java.util.*;
    public class Hand{
    
    	private ArrayList<Card> cards = new ArrayList<Card>();
    	private Card toPlay;
    	private Deck deck;
    	private final String name;
    
    	public Hand(Deck d, String n, int num){
    		deck = d;
    		name = n;
    		start(num);
    	}
    	private void start(int num){
    		for (int i=0; i<num; i++) {
    			pickUp();
    		}
    	}
    	public Card getCard(int i){
    		return (Card)(cards.get(i));
    	}
    	public int size(){
    		return cards.size();
    	}
    	public void setPlay(Card c){
    		toPlay = c;
    	}
    	public void print(){
    		for (int i=0; i<size(); i++){
    			System.out.println(cards.get(i));
    		}
    		System.out.println("\n\n");
    	}
    
    	private void pickUp(){
    		cards.add(deck.drawCard());
    	}
    
    
    	public Card play(Card topCard){
    		print();
    		// if card exists and has either the same suit or value but not both (exclusive or)
    		if(toPlay!=null&&(toPlay.getSuit()==topCard.getSuit()
    			^ toPlay.getValue()==topCard.getValue())){
    			System.out.println("Card "+toPlay+" has been played on "+topCard+"\nby "+name+"\n");
    			// return the card to be played, remove it from the hand, make toPlay null
    			int index = cards.indexOf(toPlay);
    			cards.remove(index);
    			Card temp=toPlay;
    			toPlay=null; //i don't know if this is the best thign to do
    			return temp;
    		}else{
    			//return the origional card recieved and set toPlay to null
    			pickUp();
    			toPlay=null;
    			return topCard;
    		}
    	}
    }
    Code:
    public class Player{
    
    	private final String name;
    	private Hand myHand;
    	
    	public Player(Hand hand, String n){
    		myHand=hand;
    		name=n;
    	}
    
    	public void turn(Card topCard){
    		System.out.println(name+"'s Turn: ("+myHand.size()+")");
    		for (int i=0; i<myHand.size(); i++){
    			Card temp = (Card)(myHand.getCard(i));
    			if (temp.getSuit()==topCard.getSuit()
    				||temp.getValue()==topCard.getValue()){
    				myHand.setPlay(temp);
    				break;
    			}
    		}
    	}
    
    	public final String toString(){
    		return name;
    	}
    }
    And now the pretty generic all purpose Deck and Card Classes:

    Code:
    import java.util.*;
    public class Deck{
    	ArrayList<Card> cards = new ArrayList<Card>();
    	public Deck(){
    		int i=0;
    		for (int a=0; a<13; a++) {
    			for (int b=0; b<4 ; b++) {
    				cards.add(new Card(a,b));
    				i++;
    			}
    		}
    		shuffle();
    	}
    	private void shuffle(){
    		for (int i=0; i<cards.size(); i++){
    			Card temp = (Card) cards.get(i);
    			int random = (int)(Math.random()*(cards.size()-i))+i;
    			cards.set(i,cards.get(random));
    			cards.set(random,temp);
    		} 
    	}
    	private void print(){
    		System.out.println("\n\n");
    		for (int i=0; i<cards.size(); i++){
    			System.out.println(((Card)(cards.get(i))).getName());
    		}
    	}
    	public Card drawCard(){
    		if (cards.size()==0){
    			System.out.println("OUT OF CARDS");
    			System.exit(-1);
    			return null;
    		}else{
    			Card temp = (Card) cards.get(cards.size()-1);
    			cards.remove(cards.size()-1);
    			System.out.println("Card "+temp+" drawn from deck\n");
    			return temp;
    		}
    	}
    	public void recieveCard(Card c){
    		System.out.println("Card "+c+" replaced to deck\n");
    		cards.add(0,c);
    	}
    	public static void main(String[] args) {
    		Deck d = new Deck();
    		d.print();
    		d.shuffle();
    		d.print();
    		System.out.println("\n"+d.drawCard());
    	}
    }
    Code:
    public class Card{
    	private static String[] suits = {"Hearts", "Spades", "Diamonds", "Clubs", "Red" , "Black"};
        private static String[] ranks  = {"Ace", "Two", "Three", "Four", "Five", "Six", "Seven", "Eight", "Nine", "Ten", "Jack", "Queen", "King", "Joker"};
    	private final int suit;
    	private final int value;
    	private final String name;
    
    	public Card(){
    		value=13;
    		suit=5;
    		name=ranks[value]+" of "+suits[suit];
    	}
    	public Card(int x, int y){
    		value=x;
    		suit=y;
    		name=ranks[value]+" of "+suits[suit];
    	}
    	public Card(String x, String y){
    		int i=0;
    		try{
    			i=Integer.parseInt(x);
    		} catch (NumberFormatException e){
    			for (i=0; i<ranks.length; i++) {
    				if (x.equals(ranks[i])){
    					break;
    				}
    			}
    		}
    		value=i;
    		i=0;
    		try{
    			i=Integer.parseInt(y);
    		} catch (NumberFormatException e){
    			for (i=0; i<suits.length; i++) {
    				if (y.equals(suits[i])){
    					break;
    				}
    			}
    		}
    		suit=i;
    		name=ranks[value]+" of "+suits[suit];
    	}
    
    	public String getName(){
    		return name;
    	}
    	public @Override String toString(){
    		return name;
    	}
    	public int getSuit(){
    		return suit;
    	}
    	public int getValue(){
    		return value;
    	}
    }
    Obviously you needn't read the whole thing just have a look at my opening comments.
    Also would it be possible if I couldn't have people arguing over irrelevant syntax, I'm only interested in functionality.
    The game run's as expected playing against itself, but that doesn't mean its good coding..
    Thank you :)

  2. Post #2
    Blue Member
    Dennab
    June 2009
    1,031 Posts
    not sure if it's because you used quote tags but something about the way you organise your code makes it awful to read.

    The curly braces too, just because it's java doesn't mean you have to use that horrible indenting style they like to use.
    Reply With Quote Edit / Delete Reply Windows Vista United Kingdom Show Events Agree Agree x 2Disagree Disagree x 2Dumb Dumb x 2 (list)

  3. Post #3
    Asheron's Avatar
    April 2011
    16 Posts
    Don't do

    Code:
    int numPlayers=2;
    int startNumber=7;
    ... but do

    Code:
    int numPlayers;
    int startNumber;
    
    public Game()
    {
        numPlayers = 2;
        startNumber = 7;
    }
    Variable initialisation is the job of the constructor. ( EDIT : Non-static variables, that is )
    Reply With Quote Edit / Delete Reply Windows 7 Belgium Show Events Dumb Dumb x 2 (list)

  4. Post #4

    April 2012
    10 Posts
    Don't do
    ... but do
    Yeh, i just haven't tidy'd it up much
    i was more interested in the passing of the Card objects in and out of the hands and the relationship of the hands to the players ..

    Edited:

    awful to read.
    It's to do with the code tag on FP I think, its easy to read when bunged into eclipse or sublime or even notepad++ .. anyway its all user preference and yes i know if I was making software for someone I would want to lay it out better, but that is not the issue I'm trying to deal with here :/
    Any tips on good/bad coding?

  5. Post #5
    Map in a box's Avatar
    July 2009
    7,306 Posts
    Don't do

    Code:
    int numPlayers=2;
    int startNumber=7;
    ... but do

    Code:
    int numPlayers;
    int startNumber;
    
    public Game()
    {
        numPlayers = 2;
        startNumber = 7;
    }
    Variable initialisation is the job of the constructor. ( EDIT : Non-static variables, that is )
    Whats wrong with how he has it? Setting variables like that sets them when the constructor is called

    Edited:

    not sure if it's because you used quote tags but something about the way you organise your code makes it awful to read.

    The curly braces too, just because it's java doesn't mean you have to use that horrible indenting style they like to use.
    Its a Java standard which is why its used in Java most of the time. Why avoid standards and an indention style you like to fit to someone elses opinion?