trjhtr

I'm writing a program in Java, a game called Pig. The rules are: There are two players. Each take turn to roll a 6-sided die; a player repeatedly rolls it until the player chooses to stop rolling and pass the dice to the other player, and add the sum of previous rolls to their points, or the player rolls a 1 during their turn, which all of their previous rolls are discarded and their turn ends immediately. The first to reach 100 points win. I've finished the code, tried to make it as robust as possible, and ended up with this:

import static java.lang.System.*;
import java.util.*;

public class PigDiceGame 

public static Scanner scan = new Scanner (in);
public static Random generator = new Random ();

public static int p1Score = 0;
public static int p2Score = 0;
public static int counter;

public static void main(String args)
 out.println("Welcome to Pig, the two-player dice game!");
 out.println("Player 1 go first");
 p1Mechanics ();


public static int Roll() 
 int Low = 1;
 int High = 7;
 int Roll = generator.nextInt (High - Low) + Low;
 return Roll;


public static void p1Mechanics() 
 counter = 0;
 out.println("Do you want to play this turn? (Y/N)");
 String answer = scan.nextLine ();
 answer = answer.replaceAll (" ", "") ;
 answer = answer.toUpperCase ();
 if (answer.equals("N")) 
 out.println ("Player 1 skipped their turn.");
 out.println ("Player 2's turn to roll");
 p2Mechanics ();
 
 int Roll = Roll();
 out.println ("You rolled a " + Roll);
 while (p1Score + counter < 100) 
 while (Roll != 1) 
 counter += Roll;
 out.println ("Do you want to roll more? (Y/N)");
 answer = scan.nextLine ();
 answer = answer.replaceAll (" ", "") ;
 answer = answer.toUpperCase ();
 while(!(answer.equals("Y")) && !(answer.equals("N")))
 out.println("Please enter only Y/N:");
 answer = scan.nextLine ();
 answer = answer.replaceAll (" ", "") ;
 answer = answer.toUpperCase ();
 
 if (answer.equals("N")) 
 p1Score += counter;
 out.println ("Player 1 ended their turn.");
 out.println ("P1 vs P2: " + p1Score + "-" + p2Score);
 out.println ("Player 2's turn to roll");
 p2Mechanics ();
 
 Roll = Roll ();
 out.println ("You rolled a " + Roll);

 
 if (Roll == 1) 
 counter = 0;
 
 out.println ("Player 1 rolled a '1'.");
 out.println ("P1 vs P2: " + p1Score + "-" + p2Score);
 out.println ("Player 2's turn to roll");
 p2Mechanics ();
 
 out.println("Player 1's score reached 100!");
 out.println("P1 wins by " + (p1Score - p2Score) + " points!");
 exit (0);


public static void p2Mechanics() 
 counter = 0;
 out.println("Do you want to play this turn? (Y/N)");
 String answer = scan.nextLine ();
 answer = answer.replaceAll (" ", "") ;
 answer = answer.toUpperCase ();
 if (answer.equals("N")) 
 out.println ("Player 2 skipped their turn.");
 out.println ("Player 1's turn to roll");
 p1Mechanics ();
 
 int Roll = Roll();
 out.println ("You rolled a " + Roll);
 maxScore: while (p2Score < 100) 
 while (Roll != 1) 
 counter += Roll;
 out.println ("Do you want to roll more? (Y/N)");
 answer = scan.nextLine ();
 answer = answer.replaceAll (" ", "") ;
 answer = answer.toUpperCase ();
 while(!(answer.equals("Y")) && !(answer.equals("N")))
 out.println("Please enter only Y/N:");
 answer = scan.nextLine ();
 answer = answer.replaceAll (" ", "") ;
 answer = answer.toUpperCase ();
 
 if (answer.equals("N")) 
 p2Score += counter;
 out.println ("Player 2 ended their turn.");
 out.println ("P1 vs P2: " + p1Score + "-" + p2Score);
 out.println ("Player 1's turn to roll");
 p1Mechanics ();
 
 Roll = Roll ();
 out.println ("You rolled a " + Roll);

 
 if (Roll == 1) 
 counter = 0;
 
 out.println ("Player 2 rolled a '1'.");
 out.println ("P1 vs P2: " + p1Score + "-" + p2Score);
 out.println ("Player 1's turn to roll");
 p1Mechanics ();
 
 out.println("Player 2's score reached 100!");
 out.println("P2 wins by " + (p2Score - p1Score) + " points!");
 exit (0);

I've run this code twice and swapped back and forth between two player's turns and it works correctly. I haven't had time to play a full game, though. I am looking for ways that I can improve the existing code. I never came up with any procedural decomposition before writing it so it's more like patching holes one by one. It may function, but it could probably be improved. Any help appreciated. I've been learning Java for a while now and really need some "expert" opinions.

1. Use objects! You're duplicating a ton of code because each player acts in exactly the same way.




2. Use methods! You're duplicating a ton of code because you're doing the same set of things on each input. Extract that all out to a method.




3. Localize your information better. There's no reason for scores to be global when they should be associated with the particular player. The scanner and generator should be declared in the main method, not shared between all instances of the class. What happens if somebody calls main twice? Do you really want to share the counter, the input stream, and the output stream? Also, make things as private as you can. Right now, any other class can reach into your PigGame and muck with all those public static properties you're exposing. There's no upside for you, and lots of downside.




4. Get out of the habit of calling System.exit() explicitly. It's almost always a Very Bad Idea for non-trivial code. Especially don't call it with a 0, which just means "I exited normally".

If I were to rewrite your code considering the above, my first pass would look like this:

import java.io.PrintStream;
import java.util.Random;
import java.util.Scanner;

public class PigDiceGame 

 public static void main(final String args) 
 System.out.println("Welcome to Pig, the two-player dice game!");

 final Scanner scanner = new Scanner(System.in);
 final Random random = new Random();

 final Player player1 = new Player(1, random, scanner, System.out);
 final Player player2 = new Player(2, random, scanner, System.out);

 while (true) 
 if (player1.takeTurn()) 
 System.out.println(player1.getName() + " wins!");
 outputScore(player1, player2);
 break;
 
 if (player2.takeTurn()) 
 System.out.println(player2.getName() + " wins!");
 outputScore(player2, player1);
 break;
 
 

 System.out.println(
 player1.getName() + " vs " + player2.getName() + ": "
 + player1.getScore() + " - " + player2.getScore());
 

 private static void outputScore(final Player winner, final Player loser) 
 System.out.println(winner.getName() + " wins by " + (winner.getScore() - loser.getScore()) + " points!");
 

 private static class Player 

 private final Random random;
 private final String name;
 private final Scanner scanner;
 private final PrintStream output;
 private int score;

 private Player(final int id, final Random random, final Scanner scanner, final PrintStream output) 
 this.name = "Player " + id;
 this.random = random;
 this.scanner = scanner;
 this.output = output;
 

 public boolean takeTurn() 
 this.output.println(this.name + "'s turn to roll");
 this.output.println("Do you want to play this turn? (Y/N)");
 if (this.answer().equals("N")) 
 this.output.println(this.name + " skipped their turn.");
 return false;
 

 int currentTurnScore = 0;
 do 
 final int roll = this.random.nextInt(6) + 1;
 this.output.println(this.name + " rolled a " + roll);
 currentTurnScore += roll;

 if (roll == 1) 
 currentTurnScore = 0;
 break;
 
 this.output.println(this.name + "'s score this turn is " + currentTurnScore);

 while (((this.score + currentTurnScore) < 100) && this.continueRolling());

 this.score += currentTurnScore;
 this.output.println(this.name + " ended their turn with " + this.score + " points.");

 return (this.score >= 100);
 

 public String getName() 
 return this.name;
 

 public int getScore() 
 return this.score;
 

 private String answer() 
 return this.scanner.nextLine().replaceAll(" ", "").toUpperCase();
 

 private boolean continueRolling() 
 this.output.println("Do you want to roll more? (Y/N)");
 String continueRolling = this.answer();

 while (!(continueRolling.equals("Y")) && !(continueRolling.equals("N"))) 
 this.output.println("Please enter only Y/N:");
 continueRolling = this.answer();
 

 return continueRolling.equals("Y");
 
 

A few things that caught my eye:

Roll is clunky. One private variable for the number of sides, that is set in the constructor, would be much easier to work with. A method might be overkill here, all you'll really need is one line of code.

The mechanics are duplicated. Use variables to represent the players, maybe a simple class, and pass that into the mechanics method. This makes your code much more concise and a lot easier to maintain.

It appears your mechanics code is doing too much. The communication with the players should be handled separately.

When you design a game engine like this, it is much better to keep main separate as well as the input and output streams. This gives you more options for testing as well as responding to different kinds of streams.

The game logic seems OK to me. I took your program and refactored it so the main method looks like this:

public static void main(String args) 
 out.println("Welcome to Pig, the two-player dice game!");

 Player player1 = new Player("Player 1");
 Player player2 = new Player("Player 2");

 while (true) 
 player1.play(player2);
 player2.play(player1);
 

Other stuff to fix: dice size, max score, etc. are constants. Repeated code (scan input, check end-of-game conditions) should be private methods. Score, player name, etc. are better as members. And generally, repeating code is the first indicator that you can improve things.