Results 1 to 4 of 4
  1. #1
    kathmandu is offline Member
    Join Date
    May 2013
    Posts
    18
    Rep Power
    0

    Default Please Critique My Code

    I just finished the Yahtzee assignment from the CS106a class. This was the first assignment that I actually completed on my own without looking any outside help. Everything works fine, but I would really appreciate some input as to how I can better structure my code, improve commenting, etc. Thanks.

    Java Code:
    /*
     * File: Yahtzee.java
     * ------------------
     * This program plays the Yahtzee game.
     */
    
    
    import java.util.Arrays;
    
    import acm.io.*;
    import acm.program.*;
    import acm.util.*;
    
    public class Yahtzee extends GraphicsProgram implements YahtzeeConstants {
    	
    	public static void main(String[] args) {
    		new Yahtzee().start(args);
    	}
    	
    	public void run() {
    		IODialog dialog = getDialog();
    		nPlayers = dialog.readInt("Enter number of players");
    		playerNames = new String[nPlayers];
    		for (int i = 1; i <= nPlayers; i++) {
    			playerNames[i - 1] = dialog.readLine("Enter name for player " + i);
    		}
    		display = new YahtzeeDisplay(getGCanvas(), playerNames);
    		playGame();
    	}
    
    	private void playGame() {
    		initializeScorecard();
    		runGame();
    		endGame();
    	}
    	
    	/*This method initializes the two-dimensional array used to keep track of player scores by setting all
    	 * scoring category values to -1.
    	 */
    	private void initializeScorecard(){
    		scorecard = new int [N_CATEGORIES + 1][nPlayers + 1];
    		
    		/*Sets the initial values for the first six scoring categories to -1.*/
    		for (int i = 0; i < 7; i ++){
    			for (int j = 0; j < nPlayers + 1; j++){
    				scorecard[i][j] = -1;
    			}
    		}
    		/*Sets the initial values for the final seven scoring categories to -1.*/
    		for (int i = 9; i < 16; i ++){
    			for (int j = 0; j < nPlayers + 1; j++){
    				scorecard[i][j] = -1;
    			}
    		}
    		
    	}
    	
    	/*This method runs the actual game, allowing players to roll the dice
    	 * and select categories in which they would like each turn's score
    	 * to be recorded.
    	 */
    	private void runGame(){
    		turnsRemaining = nPlayers * N_SCORING_CATEGORIES;
    		while (turnsRemaining > 0){
    			playerTurn();
    		}
    	}
    	
    	/*This method is called for each turn.  The player is allowed to roll the dice
    	 * up to three times before selecting a category in which to score the final roll.
    	 * The final roll is then scored and the score card is updated.
    	 */
    	private void playerTurn(){
    		player = currentPlayer();
    		boolean eligibleCategory = false;
    		for ( int i = 0; i < 3; i++){
    			if (i > 0){
    				display.printMessage(selectDice);
    				display.waitForPlayerToSelectDice();
    			}
    			if( i == 0)display.waitForPlayerToClickRoll(currentPlayer());
    			if (i == 0) rollDice();
    			if (i > 0) rerollDice();
    			display.displayDice(dice);
    		}
    		display.printMessage(pickCategory);
    		selectedCategory = display.waitForPlayerToSelectCategory();
    		if (scorecard[selectedCategory][player] == -1) eligibleCategory = true;
    		while (eligibleCategory == false){
    			display.printMessage(pickAgain);
    			selectedCategory = display.waitForPlayerToSelectCategory();
    			if (scorecard[selectedCategory][player] == -1) eligibleCategory = true;
    		}
    		score = scoreRoll(selectedCategory);
    		display.updateScorecard(selectedCategory, player, 
    				score);
    		scorecard[selectedCategory][player] = score;
    		turnsRemaining --;
    	}
    	
    	/*This method tabulates the final scores for each of the players and announces the winner*/
    	private void endGame(){
    		calculateUpperScores();
    		calculateLowerScores();
    		calculateTotalScores();
    	}
    	
    	/*This method calculates the sum of the scores from the top six categories for each players and
    	 * displays the sums in the upper score row on the scorecard.  If any player has earned a total upper
    	 * score of 63 or more, an upper bonus of 35 if awarded.
    	 */
    	private void calculateUpperScores(){
    		int upperScore = 0;
    		int upperBonus = 0;
    		for (int i = 1; i < nPlayers + 1; i++){
    			for (int j  = 1; j < 7 + 1; j++){
    			upperScore += scorecard[j][i];
    			}                           
    			scorecard[7][i] = upperScore;
    			if (upperScore >= 63) {
    				upperBonus = 35;
    			}
    			scorecard[8][i] = upperBonus;
    			display.updateScorecard(7, i, upperScore);
    			display.updateScorecard(8, i, upperBonus);
    			upperScore = 0;
    			upperBonus = 0;
    		}
    	}
    		/*This method calculates the sum of the scores from the bottom seven categories for each player and 
    		 * displays the sums in the lower score row on the scorecard. 
    		 */
    		private void calculateLowerScores(){
    			int lowerScore = 0;
    			for (int i = 1; i < nPlayers + 1; i++){
    				for (int j  = 9; j < 16 + 1; j++){
    				lowerScore += scorecard[j][i];
    				}                           
    				scorecard[16][i] = lowerScore;
    				display.updateScorecard(16, i, lowerScore);
    				lowerScore = 0;	
    			}
    	}
    		
    		/*This method calculates the total score for each player, determines the winner of the game, and
    		 * prints a corresponding message at the bottom of the screen. 
    		 */
    		private void calculateTotalScores(){
    			for (int i  = 1; i < nPlayers + 1; i ++){
    			scorecard[17][i] = scorecard[7][i] + scorecard[8][i] + scorecard[16][i];
    			display.updateScorecard (17, i, scorecard[17][i]);
    			}	
    			
    			int largest = scorecard[17][1];
    			int index = 0;
    			for (int i = 1; i < nPlayers + 1; i++) {
    			  if ( scorecard[17][i] > largest ) {
    			      largest = scorecard[17][i];
    			      index = i;
    			   }
    			}
    			display.printMessage("Congratulations to " + playerNames[index] + " !  You won with a total score of " + largest + ".");
    		}
    	
    	/*Determines the current player for the purpose of updating the scorecard*/
    	private int currentPlayer(){
    		int i = 0;
    		if (nPlayers == 2){
    			if (turnsRemaining % nPlayers == 0) i = 1;
    			if (turnsRemaining % nPlayers == 1) i = 2;
    		}
    		if (nPlayers == 3){
    			if (turnsRemaining % nPlayers == 0) i = 1;
    			if (turnsRemaining % nPlayers == 2) i = 2;
    			if (turnsRemaining % nPlayers == 1) i = 3;
    		}
    		else if (nPlayers == 4){
    			if (turnsRemaining % nPlayers == 0) i = 1;
    			if (turnsRemaining % nPlayers == 3) i = 2;
    			if (turnsRemaining % nPlayers == 2) i = 3;
    			if (turnsRemaining % nPlayers == 1) i = 4;
    		}
    		return i;	
    	}
    	
    	/*This method is called in order to roll the dice at the beginning
    	 * of each turn.
    	 */
    	private void rollDice(){
    		int die;
    		for (int i = 0; i < N_DICE; i++){
    			die = rgen.nextInt(1, 6);
    			dice [i] = die;
    		}
    	}
    	
    	/*This method is called after the initial dice roll, re-rolling specific
    	 * dice based on the player's selection. 
    	 */
    	private void rerollDice(){
    		int die;
    		for (int i = 0; i < N_DICE; i++){
    			die = rgen.nextInt(1, 6);
    			if (!display.isDieSelected(i))dice [i] = die;
    		}
    	}
    	
    	/*This method takes a category selected by the player as an input parameter, then calculates
    	 * and returns the score for that category.
    	 */
    	private int scoreRoll (int category){
    		int score = 0;
    		switch (category){
    		
    			case 1://Ones
    				for (int i = 0; i < N_DICE; i++){
    					if (dice [i] == 1) score ++;
    				}
    				break;
    				
    			case 2://Twos
    				for (int i = 0; i < N_DICE; i++){
    					if (dice [i] == 2) score += 2;
    				}
    				break;
    				
    			case 3://Threes
    				for (int i = 0; i < N_DICE; i++){
    					if (dice [i] == 3) score += 3;
    				}
    				break;
    				
    			case 4://Fours
    				for (int i = 0; i < N_DICE; i++){
    					if (dice [i] == 4) score += 4;
    				}
    				break;
    				
    			case 5://Fives
    				for (int i = 0; i < N_DICE; i++){
    					if (dice [i] == 5) score += 5;
    				}
    				break;
    				
    			case 6://Sixes
    				for (int i = 0; i < N_DICE; i++){
    					if (dice [i] == 6) score += 6;
    				}
    				break;
    				
    			case 9://Three of a Kind
    				int case9aMatches = 0;
    				int case9bMatches = 0;
    				int case9cMatches = 0;
    				for (int i = 1; i < N_DICE; i++){
    					if (dice [0] == dice [i]) case9aMatches ++;
    				}
    				for (int i = 2; i < N_DICE; i++){
    					if (dice [1] == dice [i]) case9bMatches ++;
    				}
    				for (int i = 3; i < N_DICE; i++){
    					if (dice [2] == dice [i]) case9cMatches ++;
    				}
    				if (case9aMatches >= 2) score += (dice[0] * 3);
    				else if (case9bMatches >= 2) score += (dice[1] * 3);
    				else if (case9bMatches >= 2) score += (dice[2] * 3);	
    				break;
    				
    			case 10://Four of a Kind
    				int case10aMatches = 0;
    				int case10bMatches = 0;
    				for (int i = 1; i < N_DICE; i++){
    					if (dice [0] == dice [i]) case10aMatches ++;
    				}
    				for (int i = 2; i < N_DICE; i++){
    					if (dice [1] == dice [i]) case10bMatches ++;
    				}
    				if (case10aMatches >= 3) score += (dice[0] * 4);
    				else if (case10bMatches >= 3) score += (dice[1] * 4);
    				break;
    				
    			case 11://Full House
    				int case11aMatches = 0;
    				int case11bMatches = 0;
    				int case11cMatches = 0;
    				for (int i = 1; i < N_DICE; i++){
    					if (dice [0] == dice [i]) case11aMatches ++;
    				}
    				for (int i = 2; i < N_DICE; i++){
    					if (dice [1] == dice [i]) case11bMatches ++;
    				}
    				for (int i = 3; i < N_DICE; i++){
    					if (dice [2] == dice [i]) case11cMatches ++;
    				}
    				if ( (case11aMatches <= 2 && case11bMatches<= 2 &&
    						case11aMatches + case11bMatches == 3) ||
    						(case11aMatches == 1 && case11cMatches == 2))score = 25;
    				break;
    				
    			case 12://Small Straight
    				for (int i = 0; i < dice.length; i++){
    					sortedDice12[i] = dice[i];
    				}
    				Arrays.sort(sortedDice12);
    				int case12aMatches = 0;
    				int case12bMatches = 0;
    				int case12cMatches = 0;
    				int case12dMatches = 0;
    				for (int i = 1; i < N_DICE; i++){
    					if (dice [0] == dice [i]) case12aMatches ++;
    				}
    				for (int i = 2; i < N_DICE; i++){
    					if (dice [1] == dice [i]) case12bMatches ++;
    				}
    				for (int i = 3; i < N_DICE; i++){
    					if (dice [2] == dice [i]) case12cMatches ++;
    				}
    				for (int i = 4; i < N_DICE; i++){
    					if (dice [3] == dice [i]) case12cMatches ++;
    				}
    						
    				/*
    				 * 12a tests determine whether or not the dice meet the criteria for a small straight
    				 */
    				boolean matchTest12a = ((case12aMatches + case12bMatches + case12cMatches + case12dMatches) <= 1);
    				
    				boolean differenceTest12a = (Math.abs(sortedDice12[0] - sortedDice12[4]) == 3);
    				
    				/*
    				 * 12b tests determine whether or not the dice meet the criteria for a large straight which also
    				 * fulfills the criteria for a small straight
    				 */
    				boolean matchTest12b = ((case12aMatches + case12bMatches + case12cMatches + case12dMatches) == 0);
    				
    				boolean differenceTest12b = (Math.abs(sortedDice12[0] - sortedDice12[4]) == 4);
    				
    				/*This statement returns "true" when the dice meet the criteria for either a small straight
    				 * or a large straight.
    				 */
    				if ((matchTest12a && differenceTest12a) || (matchTest12b && differenceTest12b) ) score = 30;
    				break;
    				
    			case 13://Large Straight
    				for (int i = 0; i < dice.length; i++){
    					sortedDice13[i] = dice[i];
    				}
    				int case13aMatches = 0;
    				int case13bMatches = 0;
    				int case13cMatches = 0;
    				int case13dMatches = 0;
    				for (int i = 1; i < N_DICE; i++){
    					if (dice [0] == dice [i]) case13aMatches ++;
    				}
    				for (int i = 2; i < N_DICE; i++){
    					if (dice [1] == dice [i]) case13bMatches ++;
    				}
    				for (int i = 3; i < N_DICE; i++){
    					if (dice [2] == dice [i]) case13cMatches ++;
    				}
    				for (int i = 4; i < N_DICE; i++){
    					if (dice [3] == dice [i]) case13dMatches ++;
    				}
    				boolean matchTest13 = ((case13aMatches + case13bMatches + case13cMatches + case13dMatches) == 0);
    				boolean difference13Test = (Math.abs(sortedDice13[0] - sortedDice13[4]) == 4);
    				if (matchTest13 && difference13Test) score = 40;
    				break;		
    				
    			case 14://Yahtzee
    				int case12matches = 0;
    				for (int i = 1; i < N_DICE; i++){
    					if (dice [0] == dice [i]) case12matches ++;
    				}
    				if (case12matches == 4) score = 50;
    				break;
    				
    			case 15://Chance
    				for (int i = 0; i < N_DICE; i++){
    					score += dice[i];
    				}
    				break;
    		}
    		return score;
    	}
    		
    /* Private instance variables */
    	private int nPlayers;
    	private String[] playerNames;
    	private YahtzeeDisplay display;
    	private RandomGenerator rgen = new RandomGenerator();
    	
    	private int turnsRemaining;//Tracks the number of turns reminaing
    	
    	private int [] dice = new int[N_DICE];//Tracks the current dice roll
    
    	private int selectedCategory;//Creates an instance variable for the category chosen by the player.
    	
    	private int player;//Creates an instance variable to track the current player.
    	
    	private int score;//Creates an instance variable the score earned during the current player turn.
    	
    	private int[] sortedDice12 = new int[5];//Creates an instance variable for an array used to score a small straight.
    	
    	private int[] sortedDice13 = new int[5];//Creates an instance variable for an array used to score a large straight.
    	
    	private int[][] scorecard;//Creates an instance variable for a two-dimensional array used to track player scores.
    	
    	/*Messages which will be displayed at the bottom of the application window.*/
    	private String selectDice = new String ("Please select which dice rolls you wish to keep.");
    	private String pickCategory = new String ("Please select a category in which to place your score.");
    	private String pickAgain = new String ("You've already recorded a score in that category.  Please chose a different one.");
    	
    	//Replace the "die" variable with testdice[i] in the rollDice and rerollDice methods for testing
    	private int[] testdice = new int[] {2,2,2,2,2};
    }

  2. #2
    DarrylBurke's Avatar
    DarrylBurke is offline Member
    Join Date
    Sep 2008
    Location
    Madgaon, Goa, India
    Posts
    11,188
    Rep Power
    19

    Default Re: Please Critique My Code

    I haven't really looked through the code, but the String constructor which takes a String parameter is (almost) never needed. In your case, it definitely isn't (lines 408-410).

    Also, when declaring and assigning an array type variable to an array literal, the new keyword can be omitted.
    Java Code:
    private int[] testdice = /*new int[]*/ {2,2,2,2,2};
    db
    If you're forever cleaning cobwebs, it's time to get rid of the spiders.

  3. #3
    jim829 is offline Senior Member
    Join Date
    Jan 2013
    Location
    United States
    Posts
    3,382
    Rep Power
    5

    Default Re: Please Critique My Code

    I noticed in your currentPlayer method you go to alot of trouble to compute the current player based on turns remaining and number of players. This results in basically,

    1,2 for two players
    1,2,3 for three players
    1,2,3,4 for four players.

    Why not just do the following
    Java Code:
       int TotalTurns = nPlayers * N_SCORING_CATEGORIES;
    
       /*Determines the current player for the purpose of updating the scorecard*/
       private int currentPlayer(){
           return (TotalTurns - turnsRemaining) % nPlayers + 1;      
       }
    Regards,
    Jim
    The Java Tutorial | SSCCE | Java Naming Conventions
    Poor planning our your part does not constitute an emergency on my part.

  4. #4
    kathmandu is offline Member
    Join Date
    May 2013
    Posts
    18
    Rep Power
    0

    Default Re: Please Critique My Code

    Quote Originally Posted by jim829 View Post
    I noticed in your currentPlayer method you go to alot of trouble to compute the current player based on turns remaining and number of players.
    I know. As I was writing the code for that section, I knew that there had to be a more simple way to go about it, but for whatever reason, it wasn't coming to me.

Similar Threads

  1. Critique my change return program
    By atac57 in forum New To Java
    Replies: 3
    Last Post: 07-15-2012, 02:25 AM
  2. Java Code Critique
    By CoderJava in forum New To Java
    Replies: 4
    Last Post: 06-30-2012, 11:14 PM
  3. Critique my first Java Program!
    By Lucid15 in forum New To Java
    Replies: 3
    Last Post: 01-26-2012, 08:56 AM
  4. Please critique
    By jim01 in forum New To Java
    Replies: 4
    Last Post: 09-24-2010, 03:43 AM
  5. Critique Java Game: Help Me Improve
    By gretty in forum New To Java
    Replies: 1
    Last Post: 07-15-2010, 04:30 AM

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •