Simple Mastermind game in OOP form

Question

I'm currently learning Java. I'm trying to get a good grasp on determining what becomes a class. I've started translating old games in to simple Java programs.

I'm looking for any critique of use of objects. Pretty much any advice as to what could make this better in the long run.

(This code has no input error checking, I know this. It should but I want to make sure I'm on the right path first.)

1. I feel that codebreaker doesn't need to be its own class.
2. I feel that the board should be a class?
3. Is there a better way than making a temp copy of my ArrayList to do the checking.

Game.java

 package com.secryption.mastermind;
 public class Game {
 private int numberOfPegs = 0; //number of pegs the player wants to use
 private String playerName = "";
 int numberOfGuesses = 0;
 boolean isAlive = true;
 public static void main(String[] args) {
 Game game1 = new Game();
 game1.setupGame();
 }
 private void setupGame() { //here is where we get the name, pegs etc. 
 ScreenHelper sh1 = new ScreenHelper(); 
 CodeBreaker cb1 = new CodeBreaker();
 setPlayerName(sh1.getUserInput("Enter your name: "));
 String pg = sh1.getUserInput("How many pegs would you like?");
 setNumberOfPegs(Integer.parseInt(pg));
 CodeMaker cm1 = new CodeMaker(numberOfPegs);
 cm1.createBoard();
 System.out.println(cm1.printBoard());
 System.out.println("You may begin guessing now. Enter your guess as colors. For example with 4 pegs.");
 System.out.println("rybl = Position 0=red, Position 1=yellow etc.");
 System.out.println("r=red, y=yellow, b=black, w=white, l=blue, g=green, p=purple");
 while(isAlive) {
 String guess = cb1.makeGuess();
 System.out.println("Your guess: " + guess);
 String result = cm1.checkGuess(guess);
 numberOfGuesses++;
 if(result.equals("BBBB")) {
 System.out.println("You win " + playerName + "! It took you " + numberOfGuesses + " to crack the code.");
 isAlive = false;
 } else {
 System.out.println("Result: " + result);
 }
 }
 }
 public void setNumberOfPegs(int i) {
 numberOfPegs = i;
 }
 public int getNumberOfPegs() {
 return numberOfPegs;
 }
 public void setPlayerName(String n) {
 playerName = n;
 }
 public String getPlayerName() {
 return playerName;
 }
 }

CodeMaker.java

 package com.secryption.mastermind;
 import java.util.ArrayList;
 public class CodeMaker {
 private int boardLength = 0;
 private String[] colors = {"r", "g", "l", "y", "p", "b", "w"};
 private ArrayList<String> board = new ArrayList<String>();
 private ArrayList<String> tempBoard = new ArrayList<String>();
 private String result = "";
 public CodeMaker(int i) {
 boardLength = i;
 }
 public int getBoardLength() {
 return boardLength;
 }
 public void createBoard() {
 for(int i = 0; i < boardLength; i++) { // get random colors for each of teh pegs
 board.add(randomColor()); // add to arraylist. just add color
 } 
 }
 private String randomColor() {
 int a = (int) (Math.random() * 7);
 String r = colors[a];
 return r;
 }
 public String printBoard() {
 for ( String s : board) {
 result = result + s; 
 }
 return result;
 }
 private void makeTempBoard() {
 tempBoard.clear();
 for( String w : board) {
 tempBoard.add(w);
 }
 }
 public String checkGuess(String g) {
 String result = "";
 String piece = "";
 makeTempBoard();
 //System.out.println(tempBoard.size());
 for(int i = 0; i < g.length(); i++) { //check each pair of guess. position/color.. should change this
 String subA = g.substring(i, i + 1);
 //System.out.println("The guess letter: " + subA);
 for(int b = 0; b < tempBoard.size(); b++) { //take each part of the guess and check it against each board peg
 //System.out.println("The tempBoad character: " + b);
 if(subA.equals(tempBoard.get(b)) && (i == b)) {
 piece = "B";
 tempBoard.set(b, "*"); //set to * so we don't get double results.
 break;
 }
 else if(subA.equals(tempBoard.get(b))) {
 piece = "W";
 //same here, remove item from arraylist. 
 tempBoard.set(b, "*");
 break;
 } else {
 piece = "-";
 } //end else
 }//end if
 result = piece + result;
 }//end for
 return result;
 }//end method
 }

CodeBreaker.java

 package com.secryption.mastermind;
 public class CodeBreaker {
 public String makeGuess() {
 ScreenHelper sh2 = new ScreenHelper();
 String guess = sh2.getUserInput("Enter a guess: ");
 return guess; 
 }
 }

ScreenHelper.java

 package com.secryption.mastermind;
 import java.io.BufferedReader;
 import java.io.IOException;
 import java.io.InputStreamReader;
 public class ScreenHelper {
 public String getUserInput(String prompt) {
 String inputLine = null;
 System.out.print(prompt + " ");
 try {
 BufferedReader is = new BufferedReader(new InputStreamReader(System.in));
 inputLine = is.readLine();
 if (inputLine.length() == 0 ) return null;
 } catch (IOException e) {
 System.out.println("IOException: " + e);
 }
 return inputLine;
 }
 }

• Emz: I also suggest you to when you've handled the feedback and processed it that you post a new revision of your code. That way you can get more focused feedback. Preferably link to this question once you do that.

Answer 1

1. CodeBreaker should not be a class. All it does can be written in one line and there is no particular responsibility that would justify to put this in an extra class:

   return (new ScreenHelper()).getUserInput("Enter a guess: ");
   

2. Why would you introduce an own class for the board? Does the board have specific properties and/or methods which should be "bundled" together? I would say: no.

3. Copying the ArrayList is fine – but: I would not make a class member for tempBoard but change the signature of makeTempBoard to e.g. ArrayList<String> createTemporaryBoardFrom(ArrayList<String>) and make it static. You really don't need an instance of this class for this operation.

---

Some more general remarks (not exhaustive):

• Separation of concerns: Your program uses System.out.println on various places spread over different classes. Why not introducing one particular class (or even better: an interface) dealing with user input and output? This could be named UserInterface and would deal with commandline for the beginning but may later be implemented with a GUI. If you use an interface here you would have to exchange only one line of code (namely where the implementing class gets instantiated) to use different frontends!
• Naming: Names as ScreenHelper or CodeMaker are not very precise. Especially any "helper" part should ring the alarm bells. "Clean Code" from Uncle Bob is a good read here.
• Try to minimize the scope of your variables: For instance isAlive. Why does it have to be a class member? Should be a local variable!

---

Minor things:

• The setupGame method does more than setup a game, it also controls the flow of events. I would put the stuff with the while loop in an extra method.
• Inlining variables: Sometimes you create variables although you don't need them, e.g. the is variable in the try block of ScreenHelpers getUserInput. Just write: inputLine = (new BufferedReader(new InputStreamReader(System.in))).readLine().

• TheEditor: Thank you, that was very helpful. So the reason that I made CodeBreaker a class, and wanted to make board one, was because everything I've read says think about the "things". Then they use some terrible example like "Person". So to me logically there is a Game, a CodeMaker, CodeBreaker and a Board. I'm still working on "what to make a class". I nee even thought about making a Interface for IO. That's a good point. I didn't like *Helper either, I'll check out that link. I saw afterward that SetupGame() did more, I need to fix that.
• TheEditor: Also, I had no idea I could write return (new ScreenHelper()).getUserInput("Enter a guess: "); like that! Being able to do that all on one line. That's handy.
• philonous: If you found my answer helpful, please also consider upvoting it ;)
• abuzittin gillifirca: Why would you introduce an own class for the board? Because it probably is a concept of the problem domain. Highlight all instances of the word board in the code, you'd see it is definitely a concept of the solution domain. Does the board have specific properties and/or methods which should be "bundled" together? Even a single property that doesn't belong should be in its own class. For example should result and board be in the same class? Whereas, board and boardLength go together. With board.size() == boardLength seems like a possible class invariant.

Answer 2

Make use of the keyword final

A final variable can only be initialized once, either via an initializer or an assignment statement. It does not need to be initialized at the point of declaration: this is called a "blank final" variable. A blank final instance variable of a class must be definitely assigned in every constructor of the class in which it is declared; similarly, a blank final static variable must be definitely assigned in a static initializer of the class in which it is declared; otherwise, a compile-time error occurs in both cases.[6] (Note: If the variable is a reference, this means that the variable cannot be re-bound to reference another object. But the object that it references is still mutable, if it was originally mutable.)

Read more here

---

List vs ArrayList

Also when you can declare the ArrayList<T> as List, that goes for Map as well. Reason being if you ever want to change which implementation it is easily done by just changing ArrayList<> to LinkedList<>, as an example.

---

Which would change lines like these

private int boardLength = 0;
private String[] colors = {"r", "g", "l", "y", "p", "b", "w"};
private ArrayList<String> board = new ArrayList<String>();
private ArrayList<String> tempBoard = new ArrayList<String>();

to

private final int boardLength;
private final String[] colors = {"r", "g", "l", "y", "p", "b", "w"};
private final List<String> board = new ArrayList<String>();
private final List<String> tempBoard = new ArrayList<String>();

---

Commenting

I would strongly advice against //end else comments like that one. As I can already track where it ends by following the code. Comments are a bit tricky to write. Most of the times they aren't needed if you are using good variable naming. In some cases however they can do wonders.

---

Redundancy

public void setPlayerName(String n) {
 playerName = n;
}
public String getPlayerName() {
 return playerName;
}

These are not needed as you can directly access the variable playerName in the Game class. When you want to change it you can simply type playerName = "str"; and System.out.println(playerName); wherever you want in your Game class.

ScreenHelper.java

This only has one method, one could argue that that method can be moved to your Game class, however I would strongly advice to just make the method static and the constructor private. That means that you do not need to instantiate ScreenHelperto use the String getUserInput(String prompt).

public class ScreenHelper {
 private ScreenHelper ();
 public static String getUserInput(String prompt) {
 String inputLine = null;
 System.out.print(prompt + " ");
 try {
 BufferedReader is = new BufferedReader(new InputStreamReader(System.in));
 inputLine = is.readLine();
 if (inputLine.length() == 0 ) return null;
 } catch (IOException e) {
 System.out.println("IOException: " + e);
 }
 return inputLine;
 }
}

Also the name ScreenHelper isn't great. Maybe let this class handle all input and output to the prompt? That would turn the class into something like this.

public class Console {
 private Console ();
 public static String userInput(String msg) {
 String inputLine = null;
 System.out.print(prompt + " ");
 try {
 BufferedReader is = new BufferedReader(new InputStreamReader(System.in));
 inputLine = is.readLine();
 if (inputLine.length() == 0 ) return null;
 } catch (IOException e) {
 System.out.println("IOException: " + e);
 }
 return inputLine;
 }
 public static void output(String msg, boolean newline) {
 System.out.print(msg);
 if (newline) {
 System.out.println(); 
 }
 }
}

CodeBreaker.java

Can easily be removed with these changes. Instead you will invoke the output and input like Console.userInput ("Enter a guess: ") and Console.output ("NotANewLine", false).