HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Simple Mastermind game in OOP form

Submitted by: @import:stackexchange-codereview··
0
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.)

  • 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 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.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:

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.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().

Code Snippets

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

Context

StackExchange Code Review Q#101114, answer score: 4

Revisions (0)

No revisions yet.