5
\$\begingroup\$

I am currently creating a Java console program for a text-based Monopoly game as a personal project. Since this is my first project that I have worked on, I would like to know what kind of object oriented programming principles and designs that I am not following. Specifically, I am wondering if I am not creating enough classes to separate functions of the program.

There are quite a few classes so I will show the most important ones here but also link to the GitHub page.

import java.util.ArrayList;
import java.util.Collections;

public class Player {
    private final String name;
    public boolean inJail = false;
    public int turnsInJail = 0;
    private int position;
    private int money = 1500;
    private int numUtilities = 0;
    private int numRailroads = 0;
    private ArrayList<Property> properties = new ArrayList<Property>();

    public Player(String name){
        this.name = name;
        position = 0;
    }

    public int getPosition() { return position; }

    public String getName() { return name; }

    public int getMoney() { return money; }

    public void addMoney(int addMoney){
        this.money += addMoney;
    }

    public void pay(Player receiving, int amount){
        receiving.addMoney(amount);
        addMoney(-amount);
    }

    public void move(int numSquares, Board board){
        position += numSquares;

        //if pass GO
        if(position >= 40){
            System.out.println(name + " passed GO and collected $200");
            money += 200;
            position %= 40;
        }

        System.out.println("Landed on " + board.getCurrentSquare(this));
        board.getCurrentSquare(this).doAction(this);
    }

    public void moveTo(int position){
        this.position = position;
    }

    //add property to Player's properties
    public void buy(Property property){
        addMoney(-property.getPrice());
        properties.add(property);
        sortPropertiesByGroup(properties);
    }

    private void sortPropertiesByGroup(ArrayList<Property> properties){
        ArrayList<Utility> utilities = new ArrayList<>();
        ArrayList<Railroad> railroads = new ArrayList<>();
        ArrayList<Property> sorted = new ArrayList<>();

        for(Property property : properties){
            if(property instanceof Utility){
                utilities.add((Utility) property);
            } else if(property instanceof Railroad){
                railroads.add((Railroad) property);
            } else {
                sorted.add(property);
            }
        }
        Collections.sort(utilities);
        Collections.sort(railroads);
        Collections.sort(sorted);

        sorted.addAll(railroads);
        sorted.addAll(utilities);
    }

    public void listProperties(){
        if(properties.isEmpty()){
            System.out.println("You do not own any properties");
        }
        for(Property property : properties){
            System.out.println(property);
        }
    }

    public int getNumRailroads(){
        int numRailroads = 0;
        for(Property p : properties){
            if(p instanceof Railroad){
                numRailroads++;
            }
        }

        return numRailroads;
    }

    public int getNumUtilities(){
        int numUtilities = 0;
        for(Property p : properties){
            if(p instanceof Utility){
                numUtilities++;
            }
        }

        return numUtilities;
    }

    //returns list of all properties that Player owns color group
    public ArrayList<ColorProperty> getOwnColorGroupList(){
        ArrayList<ColorProperty> list = new ArrayList<>();
        for(Property property: properties){
            if(property instanceof ColorProperty && ownsGroup(((ColorProperty) property).getGroup())){
                list.add((ColorProperty) property);
            }
        }
        return list;
    }

    //return list of all properties that Player can place house
    public ArrayList<ColorProperty> getHouseableProperties(){
        ArrayList<ColorProperty> houseable = new ArrayList<>();
        for(ColorProperty i : getOwnColorGroupList()){
            boolean lowestHouses = true;

            for(ColorProperty j : getOwnColorGroupList()){
                if(i.getGroup() == j.getGroup() && i.getNumHouses() > j.getNumHouses()){
                    lowestHouses = false;
                }
            }

            if(lowestHouses && i.getNumHouses() != 5){
                houseable.add(i);
            }
        }

        return houseable;
    }

    //check if property is in Player's properties
    private boolean owns(Property property){
        return properties.contains(property);
    }

    //check if Player owns all of a certain color group
    public boolean ownsGroup(ColorProperty.Group group){
        int count = 0;

        for(Property property : properties){
            if(property instanceof ColorProperty && ((ColorProperty) property).getGroup() == group){
                count++;
            }
        }

        return (count == group.maxInGroup);
    }
}
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

public class Game {

    private Jail jail = new Jail(this);
    private Dice dice = new Dice();
    private final Board board = new Board(jail, dice);
    private ArrayList<Player> players = new ArrayList<Player>();

    public void startGame(int numPlayers){

        for(int i = 1; i <= numPlayers; i++){
            System.out.print("Player " + i + " name: ");
            players.add(new Player(Input.read()));
        }

        turn(players.get(0));
    }

    //pass turn to next Player
    public void turn(Player currentPlayer){
        System.out.println("\n" + currentPlayer.getName() + "'s turn!\nMoney: $" + currentPlayer.getMoney());

        if(currentPlayer.inJail){ //if player doesn't escape jail on turn, skips to showOptions

            if(!jail.jailTurn(currentPlayer, dice, board)) {
                showOptions(currentPlayer);
            }
        } else { //if player is not in jail
            System.out.println("Position: " + board.getCurrentSquare(currentPlayer));
            int numDoubles = 0;
            do{
                currentPlayer.move(dice.roll(), board);
                numDoubles++;

                if(numDoubles == 3){
                    jail.sendToJail(currentPlayer);
                }
            } while (numDoubles < 3 && dice.isDouble());
        }

        showOptions(currentPlayer);
    }

    public void endTurn(Player currentPlayer){
        int currentIndex = players.indexOf(currentPlayer);
        if(currentIndex + 1 == players.size()){
            turn(players.get(0));
        } else {
            turn(players.get(currentIndex + 1));
        }
    }

    //player options after roll and land on a square
    private void showOptions(Player currentPlayer){
        List<PlayerOption> options = Arrays.asList(
                new ListPropertiesOption(currentPlayer),
                new BuyHouseOption(currentPlayer),
                new EndTurnOption(this, currentPlayer)
        );

        PlayerOption selectedOption = (PlayerOption) Input.selectOptions(options, "Additional Actions:");
        selectedOption.action();

        showOptions(currentPlayer); //when player does not select end turn
    }
}
public abstract class Property extends Square {
    private final int price;
    private final int rent;
    protected Player owner = null;

    public Property(String name, int price, int rent){
        super(name);
        this.price = price;
        this.rent = rent;
    }

    public Player getOwner() {
        return owner;
    }

    public int getPrice(){
        return price;
    }

    public int getRent(){
        return rent;
    }

    public void offerBuy(Player currentPlayer){
        System.out.println("Would you like to buy " + name + " for $" + price + "?");
        String response = Input.read().toLowerCase();

        if(response.contains("y")){
            currentPlayer.buy(this);
            owner = currentPlayer;
        }
    }

    @Override
    public void doAction(Player currentPlayer) {
        if(currentPlayer == owner);
            //square is owned by the currentPlayer
        else if(owner != null) {
            //square is owned by someone else
            System.out.println(currentPlayer.getName() + " paid " + owner.getName() + " $" + getRent() + " in rent");
            currentPlayer.pay(owner, getRent());
        } else {
            //square can be bought
            offerBuy(currentPlayer);
        }
    }
}

https://github.com/Dex-Max/Monopoly

\$\endgroup\$

1 Answer 1

5
\$\begingroup\$

Kudos for this decent-sized, non-trivial project. The general code quality is pretty good. Still, I have some suggestions for what could be done better, in my opinion.

I will base my code review on the GitHub repository at this commit hash: 2a8709d

I will make the suggestions for the first occurrence, but some may apply to multiple places.


Specifically, I am wondering if I am not creating enough classes to separate functions of the program.

The number of classes looks good to me, and also the size of each class. However, in my opinion the responsibilites of the classes could be separated a bit better. I will get to that.


Monopoly.java

Dependencies

The main method contains this code:

Dice dice = new Dice();
Jail jail = new Jail();
ArrayList<Player> players = createPlayers(2);
Board board = new Board(jail, dice, players);

Game game = new Game(jail, dice, board, players);
jail.setGame(game);

I like that you are injecting the dependencies (dice, jail and players) into the constructors of Game and Board. However, you are injecting almost the same dependencies into both Board and Game, which makes me think: Maybe they are both doing similar things. Whatever it is for which the game needs references to those objects, maybe the Game could just tell the Board to do it instead. We will see when we get to those classes.

Looking at the last 2 lines, the Game and the Jail directly depend on each other. Both instances hold a reference to the other one. If the Jail requires the Game for anything, maybe the Game could pass itself, or even just a callback method, as an argument when calling a method on Jail, instead of requiring a reference from the start. It also makes sure it is always in a consistent state, because the compiler enforces that you pass a Jail into the constructor of Game, but one might easily forget that jail.setGame(game) must be called, and the errors that would occur later on if you forgot it would not directly indicate what is missing.

In both cases, I would recommend to try to keep the dependencies of each class to a minimum, and to keep dependencies one-directional (i. e. A depends on B, but not the other way around), because otherwise the risk of spaghetti code might increase.

Access modifiers and interfaces

static ArrayList<Player> createPlayers(int numPlayers){

Each member (e. g. methods, fields etc.) should have the most restrictive access modifier possible. In this case, private static ... would be appropriate, to prevent unwanted method calls from other parts of the code which might lead to bugs.

For collections, like the ArrayList in this case, it is good practice to use the most abstract applicable type, and to prefer interfaces over concrete class types for return values. In this case, you could declare the return type as List<Player>, and still return an ArrayList<Player. This is even more important for public methods, but in any case it can greatly increase the flexibility of your code, because you can simply change the implementation of the method (and the returned type) without needing to change other parts of the code base which depend on that method.

Formatting/whitespace

for(int i = 1; i <= numPlayers; i++){

The whitespace around the braces and parentheses is unconventional. At least it is consistent throughout the code base (as far as I've seen), but I would recommend having a look at an "official" style guide (e. g. from Oracle, or this one from Google), and following it even if personal preferences might vary in some cases. Even if you are working alone on your own project, it will make it easier for yourself if your code looks like other peoples' code.

Nested method calls

players.add(new Player(Input.read()));

The more method calls are nested on a single line, the harder to read it becomes. Just like the computer uses a stack for those method calls, you will have to put them on your "mental stack" when reading the code. The reader's thoughts while reading that line might be similar to "So I add a player, but first I need to create a new player, for that I need to read the input to get the player's name ... and then what do I do with it again?".

Instead, I would recommend writing the code in a more "linear" way, i. e. the way you would also read a story, top to bottom, e. g.:

String playerName = Input.read();
Player player = new Player(playerName);
players.add(player);

It's slightly longer, but each line only does one thing, and you never have to jump back and forth while reading it. You can also consider creating a separate method for those lines, e. g. createPlayer, considering that this is called within the method createPlayers.


Board.java

Public fields

public Jail jail;
public Dice dice;
public ArrayList<Player> players;

Fields (i. e. instance variables) should not be public, since it exposes details of the class's implementation and makes it very difficult to change it if other classes are already depending on them. So those fields should ideally be private, and if you need to allow other classes to access those, it would be better to use a public getter method instead, but that should also be considered twice and avoided if possible.

Final

private final Square[] board = new Square[40];
private Deck communityChest = new Deck();
private Deck chance = new Deck();

Some of the fields, in this and other classes, are declared as final, but some that seem like they could also be final are not. I would recommend using final (at least for fields) whenever possible, and rather removing it consciously if something should be non-final. This way you can prevent unwanted changes, and it also communicates that they should not and don't need to be reassigned.

Responsibilites

The Board creates the Community Chest and Chance cards. This seems to be a responsibility of the card decks. I would suggest making Deck an interface, and creating two implementations, CommunityChestDeck and ChanceDeck, which both create their own cards.


public Square getCurrentSquare(Player player){
    return getSquareAt(player.getPosition());
}

There is a lot of back and forth in these 2 lines of code. The board is asked for the square on which the player is standing, but for that it needs to know from the player at which position they are standing. Whenever classes depend on each other like that, it can become very difficult to change the code without having to change it many places. It would be much simpler to just do getCurrentSquare(int position). Instead, I would recommend removing the position from the player, and only letting the board know where each player stands. Then, getCurrentSquare(Player player) can look up the position of this player by itself. Alternatively, I would consider simply removing the method, since getSquareAt(int position) is also public and can directly be called with the player's position.

Naming

private final Square[] board = new Square[40];

This array named board exists inside of the class Board. This seems counter-intuitive, since the class itself already represents the board. I would suggest renaming it to squares, since that describes exactly what it is.

Comments

//creating all squares on the board
for(int i = 0; i < 40; i++){
    board[i] = createSquare(i);
    board[i].index = i;
}

//create community chest/chance deck
for(int c = 0; c < 16; c++){
    communityChest.add(createCommunityChestCard(c));
    chance.add(createChanceCard(c));
}

These comments, each simply describing what a block of code does, could be replaced with methods. Generally, if a for-loop simply does something very simple for each element of a collection (e. g. here createSquare), you can consider creating a method with the plural form (createSquares and createCards), and thus avoid the comment. It will be easier to read, and the constructor here would become much shorter.

If you implement the methods in a way that they return the array or deck of cards instead of accessing and writing to it, you can even initialize it directly, like this: private final Square[] board = createSquares();

Error handling

default:
    return null;

This occurs when initializing the cards and squares, in the cases where the index is out of range. I would recommend throwing an IllegalArgumentException instead of returning null, because otherwise you might get a NullPointerException possibly much later, and the cause will be more difficult to find.

Possible bug

case 4:
    return new MoveToCard(new int[]{5}, this, "Take a ride on the Reading Railroad");
case 5:
case 6:
    return new MoveToCard(new int[]{5, 15, 25, 35}, this, "Advance to nearest Railroad");

The method createChanceCard contains this piece of code. Case 5 is empty, which might be intended (doing the same as for case 6), but it seems like it may have been forgotten since it is the only empty case of many cases in different methods. If it is intended to do the same as in case 6, it might be better to explicitly do the same (i. e. copy the statement from case 6), or to add a comment.


Card.java

Consistency

String text;

public Card(String message){
    this.text = message;
}

In one case it is named text, in the other it is named message. I would recommend using the same name in both cases, and message or even description is a bit clearer than text in my opinion.

Mixing UI and logic

public void play(Player player){
    doAction(player);
    System.out.println("You drew: " + text);
}

public abstract void doAction(Player player);

I would recommend against outputting text to the player from within an entity like a Card. This seems to be the responsibility of whichever class draws the card. I. e. first you draw it, then you print "You drew: ...", then you play the card (card.play(player)).

In case you remove the print statement, I would suggest removing doAction (whose name is very vague), and directly using play as your abstract void method.

Types

CollectCard declares a field protected Integer amount;. There might be a reason for using Integer which I am missing, but in most cases int should be used, mostly for simplicity, and sometimes also for performance.

Naming

It to me a moment to realize what the class CollectEveryCard actually does. It does not "collect every card", instead it "collects from everyone". I suggest renaming it to CollectFromEveryoneCard or CollectFromEveryPlayerCard, even if it is slightly longer.

Non-obvious logic

int minimumDistance = 40;

for(int i = 0; i < index.length; i++){
    int distanceToIndex = (40 + index[i] - player.getPosition()) % 40;
    if ((40 + index[i] - player.getPosition()) < minimumDistance) {
        minimumDistance = distanceToIndex;
    }
}

player.move(minimumDistance, board);

This code occurs within the class MoveToCard. It is not immediately obvious what is happening here. It might require a comment to explain it (if the actual algorithm is as complicated as it currently looks), or better, some refactoring to make it more obvious.

The first step would be to move this code into a private method whose name describes what it does, and to simply call that private method from doAction (if we did not have to override doAction, it would be better to directly rename it instead).

Contributing to the confusion is that there is an int[] named index defined in this class. Since it is an array of integers, I would assume that it probably should be renamed to indices and that each contained int is an index. However I'm not sure, and might also interpret that the whole array somehow represents an index, although I don't know how it might do that.

It seems like it moves the player to the closest position out of the given positions. If that is true, then int[] index should probably be named int[] eligibleSquarePositions (and its type should probably be changed to Iterable<int>), and distanceToIndex renamed to distanceToSquare. To make the logic more obvious, you can then extract a method private int minimumDistance(int playerPosition, Iterable<int> squarePositions).

There is also a bug: In this line if ((40 + index[i] - player.getPosition()) < minimumDistance) it seems like you forgot the % 40 that you did in the line above. This will cause some positions to never be closer than the minimumDistance of 40. What you probably want to do instead is to reuse the calculated distance from above, and do if (distanceToIndex < minimumDistance).


ColorProperty.java

Naming

private int numHouses = 0; //number of houses currently on property
private final int houseCost;

//rent based on number of houses
private final int rent1;
private final int rent2;
private final int rent3;
private final int rent4;
private final int rentH;

The comments already indicate it. These names are not very clear and require additional info in order to be understood. I suggest renaming them like this for clarity:

private int currentNumberOfHouses = 0;

private final int houseCost;
private final int rentWith1House;
private final int rentWith2Houses;
private final int rentWith3Houses;
private final int rentWith4Houses;
private final int rentWithHotel;

Long argument lists

public ColorProperty(String name, Group group, int price, int rent, int rent1, int rent2, int rent3, int rent4, int rentH){

I would recommend formatting them like this:

public ColorProperty(
    String name,
    Group group,
    int price,
    int rent,
    // etc.
){

Generally long argument lists should be avoided, but if they are necessary, then this is usually easier to read.

Switch statements

The switch statement in the constructor can be moved to a method private int houseCostByColorGroup(Group group) to keep the constructor short and clean.


public int getRent() {
    int rent = 0;
    switch(numHouses){
        case 0:
            rent = super.getRent();
            if(getOwner().ownsGroup(group)){
                rent *= 2;
            }
            break;
        case 1:
            rent = rent1;
            break;
        case 2:
            rent = rent2;
            break;
        case 3:
            rent = rent3;
            break;
        case 4:
            rent = rent4;
            break;
        case 5:
            rent = rentH;
            break;
    }

    return rent;
}

This long switch statement can be refactored and simplified by simply returning the value in each case, like this:

public int getRent() {
    switch(numHouses){
        case 0:
            if (getOwner().ownsGroup(group)) {
                return 2 * super.getRent();
            } else {
                return super.getRent();
            }
        case 1:
            return rent1;
        case 2:
            return rent2;
        case 3:
            return rent3;
        case 4:
            return rent4;
        case 5:
            return rentH;
        default:
            return 0;
    }
}

This way it is a direct mapping from one value to another value, without having to read the whole method to see if anything else changes it.

Note that I return 0 in the default case, since that is equivalent to your version, but I would strongly recommend to throw an exception instead, since it is not a valid case, and can only happen in case of a bug.

Formatting

public Group getGroup() { return group; }

public int getNumHouses() { return numHouses; }

public int getHouseCost() {
    return houseCost;
}

These methods are not formatted consistently, even though they all do basically the same. I would suggest using the formatting of getHouseCost in all cases.


Deck.java

//plays top card and puts it on bottom of deck
public void playTop(Player player){
    Card topCard = deck.removeFirst();
    add(topCard);
    topCard.play(player);
}

public void add(Card card){
    deck.addLast(card);
}

By directly calling deck.addLast(topCard) instead of add(topCard), the implementation of playTop becomes clear enough that the comment can be removed. Even as it is now, that very short 3-line method is in my opinion clearer than the comment itself.


Game.java

Recursion

It seems like each new turn is a recursive call. This could lead to a stack overflow, and the game crashing, if the game lasts enough rounds. (Maybe the compiler or the JVM optimizes it away, but I wouldn't bet on it.)

I would suggest looking into how games and game engines implement their so called "game loop", and using a game loop here as well. In short, you would have an infinite loop as long as your game is running, and process input in each iteration. You might have another infinite loop during a player's turn, which is exited when they choose the EndTurnOption.

Naming

//pass turn to next Player
public void turn(Player currentPlayer){

This comment indicates that it might be better to rename the method to passTurnToNextPlayer and remove the comment. However, the comment might even be misleading and the name already correct. It seems like this method just executes the current player's turn, and passing it to the next player is actually done in endTurn. I could imagine that the comment was above endTurn some time ago, but wasn't moved when the methods were reordered. This is why comments should generally be avoided, especially if they can be replaced by better names, and in the cases were comments are helpful, they should be maintained with the same diligence as the implementation.

Better candidates for renaming this method might be executeTurn or playTurn, since turn as a noun is meant, but turn as a verb exists (with a completely different meaning) and in this case a verb would be expected.

Unnecessary comments

if(currentPlayer.inJail){ //if player doesn't escape jail on turn, skips to showOptions
    // ...
} else { //if player is not in jail

These comments don't add any information, they just restate what the code already says. I would suggest simply removing them. However, they might indicate that the code should be refactored and the structure simplified, so you might consider e. g. extracting parts of it to separate methods. For example by extracting 2 methods, the whole thing can be simplified to this:

if (currentPlayer.inJail) {
    playJailPhase(currentPlayer, dice, board);
} else {
    playDiceRollPhase(currentPlayer, dice, board);
}
showOptions(currentPlayer);

//player options after roll and land on a square
private void showOptions(Player currentPlayer){

This comment might be true at the moment, but it explains a part of the logic that is defined somewhere else completely. If that changes in the future, the comment will be wrong and misleading. I suggest to simply remove it. The method name is very clear about what it does, and the method should not care about when it is called.


showOptions(currentPlayer); //when player does not select end turn

This comment is confusing. It is worded like a condition, but the code does not indicate that it contains such a condition. Maybe it tries to explain a piece of code that is defined somewhere else and implied here, or the code already changed and the comment wasn't updated. They way I understand the code, this call showOptions(currentPlayer) always gets executed, even if the player did select "end turn".


Input.java

Recursion

selectOptions calls itself whenever the user's input could not be parsed as a number. This could potentially lead to the game crashing if the user inputs invalid text often enough. I would recommend refactoring it to a loop.

Comments and clarity

//outputs numbered list of options and returns the selected one
public static Object selectOptions(List<?> list, String message){

The comment might actually be needed here, but only because the code is not clear enough about what it does. By changing the return type to PlayerOption, we can already remove part of the comment (and also the cast in the Game class). By replacing List<?> list with List<PlayerOption> options, we can remove the rest of the comment.


Jail.java

This class does not have any state of its own, but depends on a lot of other classes to execute its logic. I would suggest giving each player their own instance of Jail, and moving the jail-related variables and logic from the player to the jail (e. g. inJail and turnsInJail of Player). The Game would then access it via the player. This way you can avoid this "ask the jail to send this player to jail, which then tells the player they are now in jail" back-and-forth logic, by doing e. g. player.goToJail() and letting the player handle their jail stay.


Player.java

There are some public fields in this class. Other fields are private and only accessed via methods. I would recommend making all fields private.


//if pass GO
if(position >= 40){

You can avoid this comment by using a local variable:

boolean passedGo = position >= 40;
if (passedGo) {

Comments are not checked by the compiler, and often overlooked by programmers. In cases like this, code can be equally as or more expressive than a comment.


//check if property is in Player's properties
private boolean owns(Property property){
// ...

//check if Player owns all of a certain color group
public boolean ownsGroup(ColorProperty.Group group){

The comments do not help unless you look at the method definition. The first one is in my opinion perfectly clear without the comment. The second one can be renamed to make it clearer:

public boolean ownsAllPropertiesOfGroup(ColorProperty.Group group) {

or

public boolean ownsWholeGroup(ColorProperty.Group group) {

Property.java

The property knows its owner, but also the owning player knows which properties they own. This can easily lead to inconsistencies and bugs. I would recommend keeping the information at one place, possibly even outside of both classes, e. g. in a Bank. The bank could keep track of which player owns which properties, and allow look-up in both ways ("who owns this property" and "which properties does this player own").


public void doAction(Player currentPlayer) {
    if(currentPlayer == owner);
        //square is owned by the currentPlayer
    else if(owner != null) {
        //square is owned by someone else
        if(!mortgaged){
            System.out.println(currentPlayer.getName() + " paid " + owner.getName() + " $" + getRent() + " in rent");
            currentPlayer.pay(owner, getRent());
        }
    } else {
        //square can be bought
        offerBuy(currentPlayer);
    }
}

That first if statement (if(currentPlayer == owner);) with the semicolon can be confusing. I would suggest returning early instead (and removing the comment since the code is already clear):

    if (currentPlayer == owner) {
        return;
    } else {
        // ...
    }
\$\endgroup\$
2
  • \$\begingroup\$ Thank you for taking the time to take a look at my code! I have an additional question about how to handle the common dependencies that Board and Game have. I would like to move those common dependencies like Dice all to the Board, but would it be better to: 1. Create getters for the dependencies so that Game can retrieve the Dice and roll it or 2. Create a rollDice method in Board so that Game can call rollDice to roll it instead. \$\endgroup\$
    – DexMax
    Commented Sep 8, 2021 at 1:38
  • 1
    \$\begingroup\$ I would prefer the second option. It is usually better to tell objects what to do, but let them handle how to do it. In the first option, you would access their internals, and thus basically treat the Board as a "storage" for references to other objects. You could also consider removing the Dice from Game and Board. Currently, Game and Board only know about the Dice because they pass them on to other methods and constructors. Considering how simple the Dice class is, I would simply let the utilities, the player, and the jail create their own instance of Dice for each dice roll. \$\endgroup\$ Commented Sep 8, 2021 at 9:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.