Problem
This video should explain how the game works, but in simple form, it’s a game where you enter a number between 1 and 3, This number is than removed from 12, in doing so the computer will then pick a number between 1-3 based on what number you have picked, if you choose to remove 1 marble it should say that it has removed 3 marbles. This works because there are 3 lots of 4 in 12. And the maths tells it to remove what number you have chosen from 4. Because there are 3 lots of 4, you can never win, which is the whole point of the game.
Is there any way I could improve on the code?
import java.util.Scanner;
public class Main {
public static int input;
public static int marble = 12;
public static int comFirst = (int) (Math.random() * 3 + 1);
@SuppressWarnings("resource")
public static void main(String[] args) throws InterruptedException {
System.out.println("There Are 12 Marbles, You Can Only Remove Up To 3 Marbles At A Time");
System.out.println("Your Goal Is To Remove The Last Marble, Marble 1");
System.out.println("To Remove A Marble Either Enter 1, 2 or 3");
System.out.println("");
while (true) {
if (marble == 1) {
System.out.println("There Is " + marble + " Marble Remaining");
} else {
System.out.println("There Are " + marble + " Marbles Remaining");
}
Scanner scanInput = new Scanner(System.in);
input = scanInput.nextInt();
if (input > 3 || input < 1) {
System.out.println("You Can Only Remove 3 Marbles At A Time");
} else {
marble = marble - input;
Thread.sleep(250);
int cpuAnswer = 4 - input;
if (cpuAnswer == 1) {
System.out.println("The Computer Has Removed " + cpuAnswer + " Marble");
} else {
System.out.println("The Computer Has Removed " + cpuAnswer + " Marbles");
}
if (marble - cpuAnswer == 0) {
System.out.println("There Are No Marbles Remaining, The Computer Has Won!!");
System.out.println("The Computer Has Won");
return;
}
if (marble - input == 0) {
System.out.println("There Are No Marbles Remaining, You Have Won!!");
return;
}
marble = marble - cpuAnswer;
}
}
}
}
Solution
Break early instead of nested if
if (input > 3 || input < 1) {
System.out.println("You Can Only Remove 3 Marbles At A Time");
} else {
....
}
The above can be rewritten as a simple if statement with continue, this prevent the indention from creeper up at the side of the screen.
if (input > 3 || input < 1) {
System.out.println("You Can Only Remove 3 Marbles At A Time");
continue;
}
....
Placing the scanner outside the loop
Scanners use internal buffering to buffer its inputstream, this mean they may consume large amounts, instead of only the part they need for the the concurrent operation. When placing the scanner outside the loop, this also allows for the player to put in the moves much quicker after each other, an still having the program pick up every move.
Place Scanner scanInput = new Scanner(System.in); before while(true){, and wrap the scanner in a try with resources like this:
try(Scanner scanInput = new Scanner(System.in)){
while(true) {
....
}
}
This also allows you to get rid of @SuppressWarnings("resource").
Proper english doesn’t have all words title case
(all println statements)
SOme people have problems with reading sentences with every word uppercase, use the english grammar rules, make only names and the first word of a sentence starting with a uppercase.
Check the state of scanInput.hasNextInt()
You should check the state of scanInput.hasNextInt() before every number is get, this method can return false if the program is accessed by the command line and the command line return EOF, this can be triggered with ctrl + D on linux. The best way to do this is replacing the true in your while loop with the check.
Unused variables
public static int comFirst = (int) (Math.random() * 3 + 1);
At the moment, you have 1 unused variable. Either you will be using this variable for a future expansion, or this variable is left while you were working on the application.
You have won case triggers after the computer move
This case triggers after the computer has done its move. If you really planning on the player to win sometime, you should move this case before the computer makes a move.
Win cases too strict
if (marble - cpuAnswer == 0) {
if (marble - input == 0) {
While with the current implementation of your application, all wins are perfect and ending with exactly 0. If you update your implementation of the computer or player in the future, this may pose a problem in the future.
if (marble - cpuAnswer =< 0) {
if (marble - input =< 0) {
The final result would look like this:
import java.util.Scanner;
public class Main {
public static int input;
public static int marble = 12;
public static void main(String[] args) throws InterruptedException {
System.out.println("There Are 12 Marbles, You Can Only Remove Up To 3 Marbles At A Time");
System.out.println("Your Goal Is To Remove The Last Marble, Marble 1");
System.out.println("To Remove A Marble Either Enter 1, 2 or 3");
System.out.println("");
try (Scanner scanInput = new Scanner(System.in)) {
while (scanInput.hasNextInput()) {
if (marble == 1) {
System.out.println("There Is " + marble + " Marble Remaining");
} else {
System.out.println("There Are " + marble + " Marbles Remaining");
}
input = scanInput.nextInt();
if (input > 3 || input < 1) {
System.out.println("You Can Only Remove 3 Marbles At A Time");
continue;
}
marble = marble - input;
if (marble - input =< 0) {
System.out.println("There Are No Marbles Remaining, You Have Won!!");
return;
}
Thread.sleep(250);
int cpuAnswer = 4 - input;
if (cpuAnswer == 1) {
System.out.println("The Computer Has Removed " + cpuAnswer + " Marble");
} else {
System.out.println("The Computer Has Removed " + cpuAnswer + " Marbles");
}
if (marble - cpuAnswer =< 0) {
System.out.println("There Are No Marbles Remaining, The Computer Has Won!!");
System.out.println("The Computer Has Won");
return;
}
marble = marble - cpuAnswer;
}
}
}
}
Splitting the main method
By splitting the main method you archieve better code reuse.
@TheCoffeeCup has written an excellent answer for this part of the code.
To continue from @FerryBig’s answer:
His code looks like:
import java.util.Scanner;
public class Main {
public static int input;
public static int marble = 12;
public static void main(String[] args) throws InterruptedException {
System.out.println("There Are 12 Marbles, You Can Only Remove Up To 3 Marbles At A Time");
System.out.println("Your Goal Is To Remove The Last Marble, Marble 1");
System.out.println("To Remove A Marble Either Enter 1, 2 or 3");
System.out.println("");
try (Scanner scanInput = new Scanner(System.in)) {
while (scanInput.hasNextInput()) {
if (marble == 1) {
System.out.println("There Is " + marble + " Marble Remaining");
} else {
System.out.println("There Are " + marble + " Marbles Remaining");
}
input = scanInput.nextInt();
if (input > 3 || input < 1) {
System.out.println("You Can Only Remove 3 Marbles At A Time");
continue;
}
marble = marble - input;
if (marble - input =< 0) {
System.out.println("There Are No Marbles Remaining, You Have Won!!");
return;
}
Thread.sleep(250);
int cpuAnswer = 4 - input;
if (cpuAnswer == 1) {
System.out.println("The Computer Has Removed " + cpuAnswer + " Marble");
} else {
System.out.println("The Computer Has Removed " + cpuAnswer + " Marbles");
}
if (marble - cpuAnswer =< 0) {
System.out.println("There Are No Marbles Remaining, The Computer Has Won!!");
System.out.println("The Computer Has Won");
return;
}
marble = marble - cpuAnswer;
}
}
}
}
The issue with that code is that everything is in the main method. Another issue is that it doesn’t follow any OOP principle.
But first, let’s fix the code. A lot of it can be replaced with ternary operators.
1:
if (marble == 1) {
System.out.println("There Is " + marble + " Marble Remaining");
} else {
System.out.println("There Are " + marble + " Marbles Remaining");
}
Can be changed to:
System.out.println("There " + (marble == 1 ? "Is" : "Are")
+ " " + marble + " Marble" + (marble == 1 ? "" : "s")
+ " Remaining");
Ugly. Try printf:
System.out.println("There %s %d %s Remaining",
(marble == 1 ? "Is" : "Are"),
marble,
(marble == 1 ? "Marble" : "Marbles"));
2:
if (cpuAnswer == 1) {
System.out.println("The Computer Has Removed " + cpuAnswer + " Marble");
} else {
System.out.println("The Computer Has Removed " + cpuAnswer + " Marbles");
}
To:
System.out.println("The Computer Has Removed " + cpuAnswer +
(cpuAnswer == 1 ? " Marble" : " Marbles"));
3:
marble = marble - cpuAnswer;
To:
marble -= cpuAnswer;
4:
System.out.println("There Are 12 Marbles, You Can Only Remove Up To 3 Marbles At A Time");
System.out.println("Your Goal Is To Remove The Last Marble, Marble 1");
System.out.println("To Remove A Marble Either Enter 1, 2 or 3");
System.out.println("");
To:
System.out.println("There Are 12 Marbles, You Can Only Remove Up To 3 Marbles At A Timen"
+ "Your Goal Is To Remove The Last Marble, Marble 1n"
+ "To Remove A Marble Either Enter 1, 2 or 3n");
After:
import java.util.Scanner;
public class Main {
public static int input;
public static int marble = 12;
public static void main(String[] args) throws InterruptedException {
System.out.println("There Are 12 Marbles, You Can Only Remove Up To 3 Marbles At A Timen"
+ "Your Goal Is To Remove The Last Marble, Marble 1n"
+ "To Remove A Marble Either Enter 1, 2 or 3n");
try (Scanner scanInput = new Scanner(System.in)) {
while (scanInput.hasNextInput()) {
System.out.println("There %s %d %s Remaining",
(marble == 1 ? "Is" : "Are"),
marble,
(marble == 1 ? "Marble" : "Marbles"));
input = scanInput.nextInt();
if (input > 3 || input < 1) {
System.out.println("You Can Only Remove 3 Marbles At A Time");
continue;
}
marble = marble - input;
if (marble - input =< 0) {
System.out.println("There Are No Marbles Remaining, You Have Won!!");
return;
}
Thread.sleep(250);
int cpuAnswer = 4 - input;
System.out.println("The Computer Has Removed " + cpuAnswer +
(cpuAnswer == 1 ? " Marble" : " Marbles"));
if (marble - cpuAnswer =< 0) {
System.out.println("There Are No Marbles Remaining, The Computer Has Won!!");
System.out.println("The Computer Has Won");
return;
}
marble -= cpuAnswer;
}
}
}
}
Now, method separation.
First, let’s extract the two main parts into displayHelp and playGame:
import java.util.Scanner;
public class Main {
public static int input;
public static int marble = 12;
public static void main(String[] args) throws InterruptedException {
displayHelp();
playGame();
}
public void displayHelp() {
System.out.println("There Are 12 Marbles, You Can Only Remove Up To 3 Marbles At A Timen"
+ "Your Goal Is To Remove The Last Marble, Marble 1n"
+ "To Remove A Marble Either Enter 1, 2 or 3n");
}
public void playGame() {
try (Scanner scanInput = new Scanner(System.in)) {
while (scanInput.hasNextInput()) {
System.out.println("There %s %d %s Remaining",
(marble == 1 ? "Is" : "Are"),
marble,
(marble == 1 ? "Marble" : "Marbles"));
input = scanInput.nextInt();
if (input > 3 || input < 1) {
System.out.println("You Can Only Remove 3 Marbles At A Time");
continue;
}
marble = marble - input;
if (marble - input =< 0) {
System.out.println("There Are No Marbles Remaining, You Have Won!!");
return;
}
Thread.sleep(250);
int cpuAnswer = 4 - input;
System.out.println("The Computer Has Removed " + cpuAnswer +
(cpuAnswer == 1 ? " Marble" : " Marbles"));
if (marble - cpuAnswer =< 0) {
System.out.println("There Are No Marbles Remaining, The Computer Has Won!!");
System.out.println("The Computer Has Won");
return;
}
marble -= cpuAnswer;
}
}
}
}
Now extract methods into showMarbleCount, getInput, and getCPUInput.
import java.util.Scanner;
public class Main {
public static int marble = 12;
public static void main(String[] args) throws InterruptedException {
displayHelp();
playGame();
}
public void displayHelp() {
System.out.println("There Are 12 Marbles, You Can Only Remove Up To 3 Marbles At A Timen"
+ "Your Goal Is To Remove The Last Marble, Marble 1n"
+ "To Remove A Marble Either Enter 1, 2 or 3n");
}
public void playGame() {
try (Scanner scanInput = new Scanner(System.in)) {
while (scanInput.hasNextInput()) {
showMarbleCount(marble);
int input = getInput();
marble -= input;
if (marble - input =< 0) {
System.out.println("There Are No Marbles Remaining, You Have Won!!");
return;
}
Thread.sleep(250);
int cpuAnswer = getAndPrintCPUAnswer();
if (marble - cpuAnswer =< 0) {
System.out.println("There Are No Marbles Remaining, The Computer Has Won!!");
System.out.println("The Computer Has Won");
return;
}
marble -= cpuAnswer;
}
}
}
public void showMarbleCount(int marble) {
System.out.println("There %s %d %s Remaining",
(marble == 1 ? "Is" : "Are"),
marble,
(marble == 1 ? "Marble" : "Marbles"));
}
public int getInput() {
int input;
do {
input = scanInput.nextInt();
if (input > 3 || input < 1) {
System.out.println("You Can Only Remove 3 Marbles At A Time");
continue;
}
} while (true);
return input;
}
public int getAndPrintCPUAnswer(int input) {
int cpuAnswer = 4 - input;
System.out.println("The Computer Has Removed " + cpuAnswer +
(cpuAnswer == 1 ? " Marble" : " Marbles"));
return cpuAnswer;
}
}
I always like some code kata. So I tried it by myself. But first my suggestions:
- After you have the algorithm clear, think about structuring your program. The state pattern is appropriate to represent the game state in every situation.
- Extract important constants like the maximum marbles to remove in general
- Identify important pieces of calculation and extract them. Calculate the maximum marbles removable at one specific turn.
- Do not make the answer of Dr Nim dependent on the user input. This seems to be the easiest way for this case. But if you have more marbles or maybe different rules Dr Nim should evaluate the amount of marbles depending on the remaining marbles. This is a very important semantic issue.
- Avoid key words like break or continue. They will hinder you to apply refactorings like “extract method”. They break the normal control flow and let the breaking conditions be spread all over the place instead of providing a single point to look at -> the loop header/footer.
- The same with return statements. Avoid multiple return statements in long methods, especially in loops. Try to have one return statement at the end of a method.
- Provide a proper exit condition for loops. Do not break or continue loops because you mess up the control flow and spread breaking condition all over the place. Extending and refactoring the code will be difficult. Provide the exit condition in the loop header or footer, nowhere else. Other developers should all exit condition see in one place. They should not be forced to collect them through the code.
As I said, I used the state pattern to represent each state of the game. It’s verbose and it’s not perfect. But maybe you get an idea of the intention of this design decision.
import java.util.Scanner;
public class DrNim {
public static void main(String[] args) {
new DrNim().start();
}
private static final int MAXIMUM_MARBLES_TO_REMOVE = 3;
private Scanner scanner;
private int marbles;
private State state;
public synchronized void start() {
setState(new Start());
while (!(getState() instanceof End)) {
getState().execute();
}
}
private State getState() {
return state;
}
private int getMaximumMarblesToRemove() {
return this.marbles > MAXIMUM_MARBLES_TO_REMOVE ? MAXIMUM_MARBLES_TO_REMOVE : this.marbles;
}
private void setState(State state) {
this.state = state;
System.out.println(state.getStateInfo());
}
private abstract class State {
public abstract String getStateInfo();
public abstract void execute();
}
private class Start extends State {
@Override
public String getStateInfo() {
StringBuffer sb = new StringBuffer();
sb.append("There Are 12 Marbles, You Can Only Remove Up To 3 Marbles At A Timern");
sb.append("Your Goal Is To Remove The Last Marble, Marble 1rn");
sb.append("To Remove A Marble Either Enter 1, 2 or 3rn");
return sb.toString();
}
@Override
public void execute() {
scanner = new Scanner(System.in);
marbles = 12;
setState(new PlayerTurn());
}
}
private class PlayerTurn extends State {
@Override
public String getStateInfo() {
return marbles + " remaining. Players turn...";
}
@Override
public void execute() {
int marblesToRemove = scanner.nextInt();
if (marblesToRemove <= 0 || marblesToRemove > getMaximumMarblesToRemove()) {
setState(new WrongPlayerInput());
} else {
setState(new PlayerRemovesMarbles(marblesToRemove));
}
}
}
private class PlayerRemovesMarbles extends State {
private int marblesToRemove;
public PlayerRemovesMarbles(int marblesToRemove) {
this.marblesToRemove = marblesToRemove;
}
@Override
public String getStateInfo() {
if (marblesToRemove == 1) {
return "Player removes 1 marble.";
} else {
return "Player removes " + marblesToRemove + " marbles.";
}
}
@Override
public void execute() {
try {
Thread.sleep(250);
} catch (InterruptedException e) {
}
marbles = marbles - marblesToRemove;
if (marbles == 0) {
setState(new PlayerWins());
} else {
setState(new DrNimTurn());
}
}
}
private class DrNimTurn extends State {
@Override
public String getStateInfo() {
return marbles + " remaining. Dr Nims turn...";
}
@Override
public void execute() {
setState(new DrNimRemovesMarbles(calculateMarblesToRemove()));
}
private int calculateMarblesToRemove() {
return marbles % (MAXIMUM_MARBLES_TO_REMOVE + 1);
}
}
private class DrNimRemovesMarbles extends State {
private int marblesToRemove;
public DrNimRemovesMarbles(int marblesToRemove) {
this.marblesToRemove = marblesToRemove;
}
@Override
public String getStateInfo() {
if (marblesToRemove == 1) {
return "Dr Nim removes 1 marble.";
} else {
return "Dr Nim removes " + marblesToRemove + " marbles.";
}
}
@Override
public void execute() {
marbles = marbles - marblesToRemove;
if (marbles == 0) {
setState(new DrNimWins());
} else {
setState(new PlayerTurn());
}
}
}
private class WrongPlayerInput extends State {
@Override
public String getStateInfo() {
if (getMaximumMarblesToRemove() == 1) {
return "You can only remove 1 marble.";
} else {
return "You can only remove 1 to " + getMaximumMarblesToRemove() + " marbles.";
}
}
@Override
public void execute() {
setState(new PlayerTurn());
}
}
private abstract class End extends State {
}
private class PlayerWins extends End {
@Override
public String getStateInfo() {
return "There are no marbles remaining, You have won!!";
}
@Override
public void execute() {
}
}
private class DrNimWins extends End {
@Override
public String getStateInfo() {
return "There are no marbles remaining, Dr Nim has won!!";
}
@Override
public void execute() {
}
}
}