Problem
I’m a beginner in Java and want to improve and learn.
This is a followup of my previous post.
This is a game called “Acey Ducey”:
The dealer shows you two cards. You decide whether you want the dealer to pick a third card or not. If the third card is between the two cards, then you win, otherwise you lose.
The full source can be found here.
Main.java
public class Main {
public static void main(String[] args) {
PlayerInterface player = new Player(100);
DeckInterface deck = new FrenchDeck();
UserInterface ui = new CommandLineInterface();
Game game = new Game(ui, deck, player);
game.start();
}
}
CommandLineInterface.java:
public class CommandLineInterface implements UserInterface {
@Override
public void printTwoPickedCards(Card card1, Card card2) {
System.out.print("Card 1: ");
printCard(card1);
System.out.print("Card 2: ");
printCard(card2);
System.out.println("-----------------");
}
@Override
public void printThirdCard(Card card3) {
System.out.print("Card 3: ");
printCard(card3);
}
private void printCard(Card card) {
System.out.println(card.getCardNumber() + " of " + card.getCardColor());
}
@Override
public void printRemainingCards(int remainingCards) {
System.out.println(remainingCards + " left");
}
@Override
public void printEndResult(PlayerInterface player) {
System.out.println("== GAME-OVER ==");
System.out.println("Your money: " + player.getMoney());
}
@Override
public void printMoneyStatus(int money) {
System.out.println("Your current balance " + money);
}
@Override
public String getValidInput(String[] acceptableUserInput) {
String userInput;
do {
System.out.print("Please type in only these keys: ");
for (int i = 0; i < acceptableUserInput.length; i++) {
if (i > 0) System.out.print(", ");
System.out.print(acceptableUserInput[i]);
}
System.out.println();
userInput = Helper.getUserInput().toLowerCase();
} while(!Helper.isTextInputValid(userInput, acceptableUserInput));
return userInput;
}
@Override
public void printStatusOfResources(int howManyCardsLeft, int currentMoney) {
System.out.println("=================");
printRemainingCards(howManyCardsLeft);
printMoneyStatus(currentMoney);
}
}
Player.java:
public class Player implements PlayerInterface{
private int money = 0;
public Player(int money) {
if (isDepositMoneySmallerThanZero(money)) {
try {
throw new NegativeNumber("Negative amount of mon");
} catch (NegativeNumber negativeNumber) {
negativeNumber.printStackTrace();
}
}
if (isTransferredMoneyEqualsZero(money)) {
try {
throw new ZeroNumber("Initial money has to be larger than 0.");
} catch (ZeroNumber zeroNumber) {
zeroNumber.printStackTrace();
}
}
this.money = money;
}
public int getMoney() {
return money;
}
public void withdrawMoney(int amount){
if (isWithdrawMoneyExceedBalance(amount)) {
try {
throw new ExceedBalance("Not enough money");
} catch (ExceedBalance exceedBalance) {
exceedBalance.printStackTrace();
}
}
this.money -= amount;
}
private boolean isWithdrawMoneyExceedBalance(int amount) {
return amount > this.money;
}
public void depositMoney(int amount){
if (isDepositMoneySmallerThanZero(amount)) {
try {
throw new NegativeNumber("Negative amount not allowed as disposal");
} catch (NegativeNumber negativeNumber) {
negativeNumber.printStackTrace();
}
}
this.money += amount;
}
private boolean isDepositMoneySmallerThanZero(int amount) {
return amount < 0;
}
private boolean isTransferredMoneyEqualsZero(int amount) {
return amount == this.money;
}
}
FrenchCard.java:
public class FrenchDeck implements DeckInterface{
List<Card> cards = new ArrayList();
public FrenchDeck() {
for (CardSuit color : CardSuit.values()) {
for (CardNumber number : CardNumber.values()) {
Card card = new Card(number, color);
this.cards.add(card);
}
}
}
public int howManyCardsLeft() {
return this.cards.size();
}
public Card pickRandomCard() {
int randomNumber = (int)Math.floor(Math.random() * howManyCardsLeft());
return cards.remove(randomNumber);
}
}
Game.js
public class Game {
final UserInterface ui;
final DeckInterface deck;
final PlayerInterface player;
final String[] acceptableInput = {"y", "n", "q"};
public Game(UserInterface ui, DeckInterface deck, PlayerInterface player) {
this.ui = ui;
this.deck = deck;
this.player = player;
}
public void start() {
while(true) {
if (!hasEnoughResources()) {
ui.printEndResult(player);
break;
}
ui.printStatusOfResources(deck.howManyCardsLeft(), player.getMoney());
Card[] cardTupel = pick2RandomCards();
ui.printTwoPickedCards(cardTupel[0], cardTupel[1]);
String userInput = askHowUserWantsToContinue();
if (userInput.equals("y")) {
System.out.println("How much money do you want to bet?");
int playerBet = getValidBetAmount(player.getMoney());
betOnThirdCard(cardTupel, playerBet);
} else if (userInput.equals("q")) {
System.exit(1);
} else if (userInput.equals("n")) {
continue;
} else {
System.out.println("unknown selection");
}
}
}
private int getValidBetAmount(int max) {
String userInputTemp;
do {
Helper.println("Please type in integer between 0 (exclusive) and " + max + " (inclusive)");
userInputTemp = Helper.getUserInput();
} while (!validBetAmount(userInputTemp, max));
return Integer.parseInt(userInputTemp);
}
private boolean validBetAmount(String userInputTemp, int maxAmount) {
if(!Helper.inputIsNotInteger(userInputTemp)) {
return (Integer.parseInt(userInputTemp) > 0 && Integer.parseInt(userInputTemp) <= maxAmount);
}
return false;
}
private void betOnThirdCard(Card[] cardTupel, int playerBet) {
Card thirdPickedCard = deck.pickRandomCard();
ui.printThirdCard(thirdPickedCard);
if(userWins(cardTupel, thirdPickedCard)) {
System.out.println("You won!");
player.depositMoney(playerBet);
} else {
System.out.println("You lost!");
player.withdrawMoney(playerBet);
}
}
private boolean userWins(Card[] cardTupel, Card thirdPickedCard) {
int valueOfThirdCard = thirdPickedCard.getCardNumberValue();
int maxValue = Math.max(cardTupel[0].getCardNumberValue(), cardTupel[1].getCardNumberValue());
int minValue = Math.min(cardTupel[0].getCardNumberValue(), cardTupel[1].getCardNumberValue());
return (valueOfThirdCard >= minValue && valueOfThirdCard <= maxValue);
}
private String askHowUserWantsToContinue() {
Helper.println("Do you want to bet?");
return ui.getValidInput(acceptableInput);
}
private boolean hasEnoughResources() {
if (deck.howManyCardsLeft() < 3 ) {
System.out.println("Game over. Not enough cards");
return false;
}
if (player.getMoney() <= 0) {
System.out.println("Game over. Not enough money");
return false;
}
return true;
}
private Card[] pick2RandomCards() {
Card card1 = deck.pickRandomCard();
Card card2 = deck.pickRandomCard();
return new Card[]{card1, card2};
}
}
The whole project is here
Solution
missing @override
Your Player and FrenshDeck don’t have the methods they implement from the interface annotated with @override. Without the Interface class it’s impossible to tell which methods are available through the interface.
interface naming
At my job we’re forced to name the interface or abstract class as the “normal” thing, and use other names for implementing classes. For example for the player that would become:
public interface Player {
...
}
public class DefaultPlayer {
...
}
I like this style because you usually talk about the interface, not the concrete implementation anyway. That way you have a variable defined like this:
Player player = new DefaultPlayer(100);
But the biggest difference is when you receive them as a parameter for a method. For example in the contructor of a game:
public Game(UserInterface ui, Deck deck, Player player) {
At least to me this makes the code read more natural.
try throw catch
This piece of code feels really awkward::
public Player(int money) {
if (isDepositMoneySmallerThanZero(money)) {
try {
throw new NegativeNumber("Negative amount of mon");
} catch (NegativeNumber negativeNumber) {
negativeNumber.printStackTrace();
}
}
if (isTransferredMoneyEqualsZero(money)) {
try {
throw new ZeroNumber("Initial money has to be larger than 0.");
} catch (ZeroNumber zeroNumber) {
zeroNumber.printStackTrace();
}
}
this.money = money;
}
Why would you throw an exception to then immediatly catch it and continue the method normally? This makes absolutely no sense to me. Either have the constructor throw the exception and handle it elsewhere, or don’t throw the exception in the first place.
Here’s how I would write the constructor:
public Player(int money) throws IllegalArgumentException{
if (money <= 0) {
throw new IllegalArgumentException("Initial money has to be larger than 0.");
}
this.money = money;
}
The money <= 0 is already easy to understand. Putting it in it’s own checking method like you did is only useful if the logic is hard and you would put a comment there otherwise. Or if you do the throwing inside that method as well. In that last case it would look like this:
public Player(int money) throws IllegalArgumentException{
assertValidInitialAmmount(money);
this.money = money;
}
public void assertValidInitialAmmount(int money) throws IllegalArgumentException {
if (money <= 0) {
throw new IllegalArgumentException("Initial money has to be larger than 0.");
}
}
Note that such a checking method is typically prefixed with check... or assert....
For this constructor this is a bit overkill but this structure could be useful if you have an entire series of checks to do before actually doing some calculation with valid parameters. That way the method itself shows how that calculation is done without getting distracted with all the parameter checks.
console based interface
You did a pretty decent job on splitting off all user interaction into it’s own class. Having a general interface seems to imply that you might want to change to a graphical interface at some point. This is also a good thing.
The (small) problem here is that all your methods names are based on a text based interface. The easiest quickfix would be to replace all print... prefixes with something more general like show....
Actually generalising for a graphical interface is a lot harder and is out of scope of this answer.
player options
There’s some unfriendly behaviour in this piece of code:
String userInput = askHowUserWantsToContinue();
if (userInput.equals("y")) {
System.out.println("How much money do you want to bet?");
int playerBet = getValidBetAmount(player.getMoney());
betOnThirdCard(cardTupel, playerBet);
} else if (userInput.equals("q")) {
System.exit(1);
} else if (userInput.equals("n")) {
continue;
} else {
System.out.println("unknown selection");
}
If the user had typed Y instead of y the game would tell him "unknown selection" and then treat it the same as if the user had chosen n. Why not make sure we get a recognised input in the first place?
The cleanest (but maybe overdesigned) solution is to create an enum with the (currently) 3 possible choices. And have the askHowUserWantsToContinue() method return one of those 3 enum values. In that method, if the user input was wrong you should just ask the user again until you get a valid input.
user interface part 2
There still seems to be plenty of System.out.println in classes outside of the user interface class. If you ever change to a graphical interface these don’t help anymore. Either decide your program is only going to be console based (and reduce the user interface overdesigning) or handle ALL user interaction through user interface classes.
Flashback time, again 🙂
Looks quite good. The code looks like you really wanted to make code good looking – which maybe is more important than people would think.
Naming
- Usually, interfaces do not have an
Interfacesuffix, but the implementation has aImplsuffix. It’s usually decided by the developing team, but I rarely see theInterfacesuffix. printMoneyStatus: I’d go withprintMoneyBalance, to make it more clear.printStatusOfResources: I’m also not sure about this one, resources is not very clear. MaybeprintPlayerStatus?- Exceptions: Always add
Exceptionas suffix to your exceptions.NegativNumberorZeroNumbersounds more like a value type. isWithdrawMoneyExceedBalance: MmmmmaybemoneyAmountExceedsBalance?depositMoney: Usually, you deposit money on an account, don’t you? MaybeaddMoneyAmount? So it readsplayer.addMoneyAmount.howManyCardsLeft(): This one misses thegetprefix.getRemainingCardAmountmaybe?hasEnoughResources: Same asprintStatusOfResources. Resources is not very clear.cardTupel: Tupel is actually a term found in ERD/databases (represents a row of a table). Since it’s an array, it should be named...cards. The s in the end is imo very important.askHowUserWantsToContinue: This is a hard one. I don’t like it, but I can’t think if something better.betOnThirdCard: The parametercardTupelisn’t very clear (beside the tupel thing). It’s the actual cards of the player, so it should be named correspondingly.
In general, I really don’t like the naming “two”, in “two first picked cards” and “third”, in “third picked card”. But since it’s the essential of the game, I’m very fine with that.
Code Flow
Always try to tell the summary of the story in one method. What I’ve seen in Game is, that the main flow will be “moved” to subroutines: When you are in Game.start, the main flow moves to betOnThirdCard: This method now checks, if the user has won and deposits/withdraws money. This is bad, because you have – or at least I have – a hard time to understand what the start method does “in general”, because I have jump around methods. The less code you need, to tell the reader what the method/type does, the faster one can understand. So I’d move the “win” check and the withdrawing to the start method.
code duplication
The printing of the cards can be simplified. The pattern is “Card ” + cardNumber, so maybe add a int cardNumber to the printCard method.
SRP
The validation in the CommandLineInterface.getValidInput(): This belongs to the “domain logic”. If you switch the UserInterface, this validation will be duplicated and has to be pulled out.
Exception handling
In the Player constructor for instance, you throw an exception and catch it directly. That’s not how it’s supposed be done. First of all, you just print the exception, but the game flow continues. The caller of the method does not know, if somethings wrong.
In general, what I would suggest, you validate the money before you instantiate the Player. If there’s still a wrong value passed, you throw a RuntimeException “upwards“. If that happens, you introduced a bug somewhere, and the game itself is not runnable anyway.
Other
CommandLineInterface.getValidInput if: Always use brackets, also for oneliners.isTransferredMoneyEqualsZero: Uh, you check ifamountis the same asmoney. Not sure if that’s correct, but at least the method name should be changed.FrenchDeck: Shouldn’t the methods have an@Overrideannotation?- You can add a static imports for
outinSystem.out, theCommandLineInterfacewould maybe look a bit more tidier. Or, maybe even better, provide a staticprintmethod and delegate the message toSystem.out.
Hope that helps.