Results 1 to 4 of 4
Thread: Please Critique My Code
 06282013, 06:51 PM #1Member
 Join Date
 May 2013
 Posts
 18
 Rep Power
 0
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 twodimensional 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, rerolling 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 twodimensional 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}; }
 06282013, 07:11 PM #2
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 408410).
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};
If you're forever cleaning cobwebs, it's time to get rid of the spiders.
 06282013, 08:03 PM #3Senior Member
 Join Date
 Jan 2013
 Location
 United States
 Posts
 3,382
 Rep Power
 5
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; }
JimThe Java™ Tutorial  SSCCE  Java Naming Conventions
Poor planning our your part does not constitute an emergency on my part.
 06282013, 10:02 PM #4Member
 Join Date
 May 2013
 Posts
 18
 Rep Power
 0
Similar Threads

Critique my change return program
By atac57 in forum New To JavaReplies: 3Last Post: 07152012, 02:25 AM 
Java Code Critique
By CoderJava in forum New To JavaReplies: 4Last Post: 06302012, 11:14 PM 
Critique my first Java Program!
By Lucid15 in forum New To JavaReplies: 3Last Post: 01262012, 08:56 AM 
Please critique
By jim01 in forum New To JavaReplies: 4Last Post: 09242010, 03:43 AM 
Critique Java Game: Help Me Improve
By gretty in forum New To JavaReplies: 1Last Post: 07152010, 04:30 AM
Bookmarks