Original question can be found here
I took the advice I got to heart, and re-wrote it. Once again I'd appreciate any advice as to how I can improve this using best practices.
Jar.java
package com.tn.jar;
public class Jar {
private String itemName;
private int numberOfItems;
private int numberToGuess;
private int numberOfGuesses;
/*
* Default constructor
*/
public Jar() {
this.numberOfGuesses = 0;
}
/*
* Getters
*/
public String getItemName() {
return itemName;
}
public void setItemName(String itemName) {
this.itemName = itemName;
}
public int getNumberOfItems() {
return numberOfItems;
}
public void setNumberOfItems(int numberOfItems) {
this.numberOfItems = numberOfItems;
}
public int getNumberToGuess() {
return numberToGuess;
}
/*
* Setters
*/
public void setNumberToGuess(int numberToGuess) {
this.numberToGuess = numberToGuess;
}
public int getNumberOfGuesses() {
return numberOfGuesses;
}
public void setNumberOfGuesses(int numberOfGuesses) {
this.numberOfGuesses = numberOfGuesses;
}
public void incrementNumberOfGuesses() {
numberOfGuesses++;
}
}
Prompter.java
package com.tn.jar;
import java.util.Random;
import java.util.Scanner;
public class Prompter {
private Jar jar;
private Scanner scanner;
/*
* Public constructor
*/
public Prompter(Jar jar) {
this.jar = jar;
this.scanner = new Scanner(System.in);
}
/*
* Method that actually starts the game
*/
public void play() {
adminSetup();
playerSetup();
}
/*
* Admin setup
*/
private void adminSetup() {
printTitle("Administrator setup");
setupGame();
}
/*
* Player setup
*/
private void playerSetup() {
printTitle("Player");
gameExplanation();
areYouReady();
getGuess();
}
/*
* Setup game.
* The user is prompted with a couple of questions for filling the jar instance.
*/
private void setupGame() {
String itemName = askQuestion("Name of items in the jar: ");
String numberOfItems = askQuestion("Maximum of lentils in the jar: ");;
while(isNan(numberOfItems)) {
numberOfItems = askQuestion("Maximum of lentils in the jar: ");
}
jar.setItemName(itemName);
jar.setNumberOfItems(Integer.parseInt(numberOfItems));
jar.setNumberToGuess(new Random().nextInt(Integer.parseInt(numberOfItems)) + 1);
}
/*
* User must confirm whether or not he is ready to play before the game starts.
*/
private void areYouReady() {
do {
System.out.print("Ready? (press ENTER to start guessing): ");
} while (scanner.nextLine().length( ) > 0);
}
/*
* The user can guess the answer. It will loop until the answer is correct.
*/
private void getGuess() {
do {
System.out.print("Guess: ");
jar.incrementNumberOfGuesses();
} while(guess() != jar.getNumberToGuess());
printResult();
}
/*
* Utility method for getting the users guess
*/
private int guess() {
String answer = scanner.nextLine();
while(isNan(answer)) {
System.out.print("Guess: ");
answer = scanner.nextLine();
}
return Integer.parseInt(answer);
}
/*
* Some explanation before the game starts.
*/
private void gameExplanation() {
System.out.printf("Your goal is to guess how many lentils are in the jar. Your guess should be between 1 and %d%n", jar.getNumberOfItems());
}
/*
* Utility method that accepts input.
*/
private String askQuestion(String question) {
System.out.print(question);
String result = scanner.nextLine();
return result;
}
/*
* Utility method for printing out headers
*/
private void printTitle(String title) {
System.out.printf("%n%s%n=========================%n%n", title.toUpperCase());
}
/*
* When the user answers correctly, he recieves the game stats.
*/
private void printResult() {
System.out.printf("%nCongratulations - you guessed that there are %d" +
" lentils in the jar! It took you %d" +
" guess(es) to get it right.%n", jar.getNumberToGuess(), jar.getNumberOfGuesses());
}
/*
* Utility method for checking if the string read by the system can be parsed as an integer
*/
private static boolean isNan(String string) {
try {
Integer.parseInt(string);
} catch(NumberFormatException nfe) {
return true;
}
return false;
}
}
Game.java
package com.tn.jar;
public class Game {
public static void main(String[] args) {
Jar jar = new Jar();
Prompter prompter = new Prompter(jar);
prompter.play();
};
}
1 Answer 1
Now this is a lot easier to follow than your previous version. Though, there are still a few things.
/*
* Getters
*/
If you require such comments to separate your classes into regions, you're doing something wrong.
Personally, I like to have the following layout and order to my class:
- Constants
- Static members
- Members
- Static constructor
- Constructor
- Static methods
- Methods
- Subclasses
And within all these categories everything is sorted alphabetically. Now many people will frown when I say "sort your stuff alphabetically", because many prefer to sort it logically, which means that the first used functions are first. I prefer alphabetically because there is never a doubt where a function is and you can easily navigate the whole file (you're seeing a function named "measure" and you know that "getValues" is above it and "setName" is below it).
But that is my personal preference.
/*
* Utility method for checking if the string read by the system can be parsed as an integer
*/
private static boolean isNan(String string) {
You obviously misunderstood what Javadoc is. It is not only commenting above the functions, it is associating documentation with a function. Well formed Javadoc would look this:
/**
* Checks if the given {@link String} is not a number.
* <p>
* The given {@link String} is considered to be not a number if
* the {@link Integer#parseInt} function fails on it.
*
* @param string The string to check.
* @return {@code true} if the given value is not a number.
* @see Integer#parseInt
*/
private static boolean isNan(String string) {
Note that tow asterisks which indicate that this is not a comment, but a Javadoc block. From this countless tools and IDEs are able to associate the Javadoc block with the function below and it automatically provide and generate documentation for this function.
Prompter
is not only prompting, it is doing everything. I therefor suggest that you rename Game
to Main
, and Prompter
to Game
.
Even though your naming is great, it is a little bit off from time to time. For exmaple:
adminSetup
does not setup something that has to do with administration tasks, it initializes the game.playerSetup
does not only setup the player, it also starts the game.getGuess
is not only getting a guess, it loops until the guess is correct.
Functions should be named after what they do, and they should only do that one thing.
String numberOfItems = askQuestion("Maximum of lentils in the jar: ");;
while(isNan(numberOfItems)) {
numberOfItems = askQuestion("Maximum of lentils in the jar: ");
}
jar.setItemName(itemName);
jar.setNumberOfItems(Integer.parseInt(numberOfItems));
Your askQuestion
function does read and return a String
, which you then try to parse as int and then make an int. It would be a lot better if you have separate functions, like this:
private String promptForString(String prompt);
private int promptForInt(String prompot);
Which would shorten your code to:
jar.setItemName(promptForString("Name of items in the jar: ");
jar.setNumberOfItems(promptForInt("Maximum of lentils in the jar: ");
The Scanner
class does have multiple different read*
methods, have a look at and use them.
String itemName = askQuestion("Name of items in the jar: ");
String numberOfItems = askQuestion("Maximum of lentils in the jar: ");;
Again, my personal preference: I hate that aligning of statements, I have a hard time reading such code because my brain switches into some sort of "column mode", and I have a hard time associating the values with the names. But that might just be me.
Also, it is a lot of work to maintain.
Otherwise this feels like an improvement to me. It feels like a lot less code which is easier to follow.
-
\$\begingroup\$ Thanks again. Such a good way to learn. If I post (yet another) version later today - do you think you one again could give some advise? \$\endgroup\$Nilzone-– Nilzone-2016年07月10日 14:42:33 +00:00Commented Jul 10, 2016 at 14:42
Explore related questions
See similar questions with these tags.