patternjavaMinor
Simple Mastermind game in OOP form
Viewed 0 times
simplemastermindgameformoop
Problem
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.)
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.equa
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.)
- I feel that codebreaker doesn't need to be its own class.
- I feel that the board should be a class?
- Is there a better way than making a temp copy of my
ArrayListto 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.equa
Solution
-
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:
-
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.
-
Copying the ArrayList is fine – but: I would not make a class member for
Some more general remarks (not exhaustive):
Minor things:
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: ");-
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.
-
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 createTemporaryBoardFrom(ArrayList) 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.printlnon 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 namedUserInterfaceand 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
ScreenHelperorCodeMakerare 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
setupGamemethod 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
isvariable in thetryblock ofScreenHelpersgetUserInput. Just write:inputLine = (new BufferedReader(new InputStreamReader(System.in))).readLine().
Code Snippets
return (new ScreenHelper()).getUserInput("Enter a guess: ");Context
StackExchange Code Review Q#101114, answer score: 4
Revisions (0)
No revisions yet.