Strategy advisor for a Yahtzee Roll

Question

I'm implementing a Yahtzee app and i have a class StrategicAdvisor that is responsible for deciding which strategy should be followed. The StrategicAdvisor has some tools: a RollAnalyzer which provides information about the current roll and a BoardAnalyzer which can tell us about the Fields vaccant (or occupied) in the game board (as well as the amount of eyes required for the bonus and other infos about the board).

The Strategy just tells the player (either AI player or human) which die should be best kept to maximize on the next roll - or if the result already is optimal to stop rolling and write immediately into the board.

Note:

The StrategicAdvisor just gives Hints on which die should be kept; there is another Advisor, namely the WriteAdvisor who decides which field is optimal at the end of rolling to be written at.

Update / Note 2

the Strategy represents the Strategy for the current Roll - it is not applicatable for the Strategy of the AI-Player for this i had the idea to implement different StrategyAdvisors, e.g. AggressiveStrategicAdvisor or DefensiveStrategicAdvisor... i'm not there yet so the current StrategicAdvisor works a kind of BasicStrategyAdvisor (right now i cannot yet make proper estimates onthe value of a Strategy)

Question here is:

Clean Code: My StrategicAdvisor has a lot of complexity and i can't find a nice way to reduce complexity here...

public class StrategyAdviser {

private final YahtzeeGame yahtzeeGame;

public StrategyAdviser(YahtzeeGame yahtzeeGame) {
    this.yahtzeeGame = yahtzeeGame;
}

public Strategy getAdvise() {
    Roll roll = yahtzeeGame.getRoll();
    Board board = yahtzeeGame.getBoard();
    YahtzeePlayer currentPlayer = yahtzeeGame.getCurrentPlayer();

    StrategyFactory factory = new StrategyFactory();
    if (!roll.hasRoll()) {
        return factory.createStartRollStrategy();
    }

    if (!yahtzeeGame.canRoll()) {
        return factory.createWriteToBoardStrategy();
    }

    RollAnalyze rollAnalyze = new RollAnalyze(roll);
    BoardAnalyze boardAnalyze = new BoardAnalyze(board, currentPlayer);
    int amountOfIdenticals = rollAnalyze.getAmountOfIdenticals();
    int highestEyeOfIdenticals = rollAnalyze.getHighestEyeOfIdenticals();
    boolean has6 = rollAnalyze.hasSix();
    boolean has5 = rollAnalyze.hasFive();
    boolean is6Empty = boardAnalyze.isTopRowEmpty(6);
    boolean is5Empty = boardAnalyze.isTopRowEmpty(5);
    boolean has3InRow = rollAnalyze.hasThreeInRow();
    boolean has4InRow = rollAnalyze.hasFourInRow();
    boolean hasBottomVariables = boardAnalyze.hasBottomRowsWithVariableCounter();

    if (amountOfIdenticals == 5) {
        return factory.createWriteToBoardStrategy();
    }

    if ((amountOfIdenticals >= 3 && boardAnalyze.isTopRowEmpty(highestEyeOfIdenticals))) {
        return factory.createStrategyRollForThreeOrMoreIdentical(highestEyeOfIdenticals, rollAnalyze);
    }
    if (amountOfIdenticals >= 2 && highestEyeOfIdenticals >= 4) {
        return factory.createStrategyRollForThreeOrMoreIdentical(highestEyeOfIdenticals, rollAnalyze);
    }

    if (has4InRow) {
        return factory.createStrategyRollForMajorStreet();
    }

    if (has6 && is6Empty) {
        return factory.createStrategyRollForSix(rollAnalyze);
    }

    if (((!has6) && (has5 && is5Empty))) {
        return factory.createStrategyRollForSix(rollAnalyze);
    }

    if (hasBottomVariables) {
        if (has6) {
            return factory.createStrategyRollForSix(rollAnalyze);
        }
        if (has5) {
            return factory.createStrategyRollForFive(rollAnalyze);
        }
    }

    if (has3InRow) {
        if (has5) {
            return factory.createStrategyRollForMinorStreet(3, 4, 5);
        } else {
            return factory.createStrategyRollForMinorStreet(2, 3, 4);
        }
    }

    return factory.createStrategyReRollAll();
}

I could write some Methods to simplify the statements but i think that would only slighlty decrease complexity... (e.g. instead of amountOfIdenticals == 5 write a method boolean hasFiveIdenticals(rollAnalyze ra);

Update

here is the intercace Strategy: There are two abstract implementations in the StrategyFactory for easier creation of Strategys.

public interface Strategy {

    boolean adviseToContinueRolling();

    void apply(YahtzeeGame game);

}

and StrategyFactory:

public class StrategyFactory {

    private static final Logger LOGGER = LoggerFactory.getLogger(Strategy.class);

    public Strategy createStrategyRollForThreeOrMoreIdentical(int highestEyeOfIdenticals, RollAnalyze rollAnalyze) {

        return new ContinueStrategy() {

            @Override
            public void apply(YahtzeeGame game) {
                LOGGER.debug("apply strategy: roll, try to get as much identicals of {} as possible", +rollAnalyze.getHighestEyeOfIdenticals());
                LOGGER.debug("based on roll: {}", rollAnalyze.getRoll());
                roll(game, rollAnalyze.getKeepingFor(highestEyeOfIdenticals));

            }

            @Override
            public String toString() {
                return "roll for identicals of " + highestEyeOfIdenticals;
            }

        };
    }


    public Strategy createStartRollStrategy() {
        return new ContinueStrategy() {

            @Override
            public void apply(YahtzeeGame game) {
                LOGGER.debug("apply strategy: roll for the first time...");
                roll(game);
            }

            @Override
            public String toString() {
                return "roll for the first time...";
            }
        };


    }

    public Strategy createWriteToBoardStrategy() {
        return new StopStrategy() {

            @Override
            public void apply(YahtzeeGame game) {
                LOGGER.debug("apply strategy: write to board... do NOT roll anymore");
            }

            @Override
            public String toString() {
                return "do not roll - write it into the board!";
            }
        };
    }

    public Strategy createStrategyRollForMajorStreet() {
        return new ContinueStrategy() {

            @Override
            public void apply(YahtzeeGame game) {
                LOGGER.debug("apply strategy: roll, try to get as much different since we have alreadey minor straight");
                roll(game, new Keeping(new HashSet<>(Arrays.asList(2, 3, 4, 5))));
            }

            @Override
            public String toString() {
                return "roll for a major street";
            }
        };
    }

    public Strategy createStrategyRollForSix(RollAnalyze rollAnalyze) {
        return createStartRollForEye(6, rollAnalyze);
    }

    public Strategy createStrategyRollForFive(RollAnalyze rollAnalyze) {
        return createStartRollForEye(5, rollAnalyze);
    }

    private Strategy createStartRollForEye(int eye, RollAnalyze rollAnalyze) {
        return new ContinueStrategy() {

            @Override
            public void apply(YahtzeeGame game) {
                Keeping keeping = rollAnalyze.getKeepingFor(eye);
                LOGGER.debug("apply strategy: roll, try to get as much identicals of {} as possible", +eye);
                LOGGER.debug("based on roll: {}", keeping);
                roll(game, keeping);
            }

            @Override
            public String toString() {
                return "roll one certain eye: " + eye;
            }
        };
    }

    public Strategy createStrategyRollForMinorStreet(Integer... eyes) {
        return new ContinueStrategy() {

            @Override
            public void apply(YahtzeeGame game) {
                LOGGER.debug("apply strategy: roll, try to get as much different since we have alreadey 3/4 of minor straight");
                roll(game, new Keeping(new HashSet<>(Arrays.asList(eyes))));
            }

            @Override
            public String toString() {
                return "roll for a minor street";
            }
        };
    }

    public Strategy createStrategyReRollAll() {
        return new ContinueStrategy() {

            @Override
            public void apply(YahtzeeGame game) {
                LOGGER.debug("apply strategy: reRoll all...");
                roll(game);
            }

            @Override
            public String toString() {
                return "reRoll all... ";
            }
        };
    }


    private abstract class TemplateStrategy implements Strategy {

        private final boolean adviseToContinue;

        private TemplateStrategy(final boolean adviseToContinue) {
            this.adviseToContinue = adviseToContinue;
        }

        @Override
        public boolean adviseToContinueRolling() {
            return adviseToContinue;
        }

        void roll(YahtzeeGame yahtzeeGame, Keeping keeping) {
            LOGGER.debug("set keeping: {}", keeping);
            yahtzeeGame.setKeepings(keeping);
            yahtzeeGame.roll();
            LOGGER.debug("...done with set Keeping and rolling...");
        }

        void roll(YahtzeeGame yahtzeeGame) {
            yahtzeeGame.roll();
            LOGGER.debug("...done, rolling...");
        }
    }

    private abstract class StopStrategy extends TemplateStrategy {

        private StopStrategy() {
            super(false);
        }

    }

    private abstract class ContinueStrategy extends TemplateStrategy {

        private ContinueStrategy() {
            super(true);
        }

    }
}

further note:

a Keeping is a set of Numbers, which tells the YahtzeeGame which die should be kept (so it's a Keeping) and wich shall be re-rolled

if you want to dig deeper into the code, it can be found under https://github.com/martinFrank/yatzee but please don't look too close - i fear i might get kicked out of this board, hahaha =)

Update

in hindsight of the progress of the question i think i should as well add the AiPlayer's turn

public class YahtzeePlayer extends BasePlayer<YahtzeeGame> {

    private static final Logger LOGGER = LoggerFactory.getLogger(YahtzeePlayer.class);


    YahtzeePlayer(String name, int color, boolean isHuman) {
        super(name, color, isHuman);
    }

    @Override
    public void performAiTurn() {

        YahtzeeGame yahtzeeGame = getBoardGame();//weg damit
        StrategyAdviser strategyAdviser = new StrategyAdviser(yahtzeeGame);
        Strategy currentStrategy = strategyAdviser.getAdvise();
        while (currentStrategy.adviseToContinueRolling()) {
            currentStrategy.apply(yahtzeeGame);
            currentStrategy = strategyAdviser.getAdvise();
        }

        WriteAdviser writeAdviser = new WriteAdviser(
                yahtzeeGame.getRoll(), yahtzeeGame.getBoard(), this, new RollAnalyze(yahtzeeGame.getRoll()));
        RowType rowType = writeAdviser.getOptimalRow();

        if (!yahtzeeGame.getBoard().getRow(rowType, this).isEmpty()) {
            throw new IllegalStateException("error - writing into an already filled row");
        }

        LOGGER.debug("write roll {} into rowType {}", yahtzeeGame.getRoll(), rowType);
        yahtzeeGame.write(rowType);


        LOGGER.debug("done Ai turn");
        yahtzeeGame.endPlayersTurn();
        yahtzeeGame.startPlayersTurn();
    }

}

Answer 1

First of all - I do not know the game and just want to point out some code related things. Thanks :)

---

Not a Getter

From the JavaBeans naming conventions:

By default, properties are determined from get/set access method combinations that follow a prescribed naming convention.
[...]

public int getMood() {
 return mood;
}

The name of the method getAdvise supposes that this will return a property but it calculates the Strategy - I would rename the method to predict. So the API could look like:

StrategyAdviser strategyAdviser = new StrategyAdviser(game);
Strategy strategy = strategyAdvise.predict();

---

RollAnalyze and BoardAnalyze

I can't justify following with resources and maybe it is just my personal experience:

RollAnalyze rollAnalyze = new RollAnalyze(roll);
BoardAnalyze boardAnalyze = new BoardAnalyze(board, currentPlayer);

The rollAnalyze and boardAnalyze can't be reused because they only work for roll, board and currentPlayer. So we need to instantiate on each call on getAdvise new objects of them.

It could be bette, if we can create instance variables of them and call the methods on them with passing in the arguments they need:

RollAnalyze rollAnalyze;
BoardAnalyze boardAnalyze;

// constuctor

public Strategy getAdvise() {
// ...

  boolean has6 = rollAnalyze.hasSix(roll);
  boolean has5 = rollAnalyze.hasFive(roll);
}

---

Instance Variable

The StrategyFactory should be a instance variable of StrategyAdviser that gets injected in by the constructor.

One benefit of a Factory is the possibility to change it dynamically. Currently it is hard coded in side your logic without the possibility to change the behavior dynamically on runtime.

---

Factory Pattern

From Wikipedia:

The Factory Method design pattern describes how to solve such problems:

• Define a separate operation (factory method) for creating an object.
• Create an object by calling a factory method

In the UML-Diagram you can see that there exists in the Creator1 only one method - the factory method.

Currently the StrategyFactory provides for each possible Strategy a method. Instead it should have only one method and it decides witch object will be returned.

There for we need to paste the if-branches from getAdvise into StrategyFactory:

public class StrategyFactory {

    RollAnalyze rollAnalyze;
    BoardAnalyze boardAnalyze;

    // constructor

    public Strategy build(YahtzeeGame yahtzeeGame) {
        Roll roll = yahtzeeGame.getRoll();
        Board board = yahtzeeGame.getBoard();
        YahtzeePlayer currentPlayer = yahtzeeGame.getCurrentPlayer();

        if (!roll.hasRoll()) {
            return createStartRollStrategy();
        }

        if (!yahtzeeGame.canRoll()) {
            return createWriteToBoardStrategy();
        }

        if (rollAnalyze.getAmountOfIdenticals(roll) == 5) {
            return createWriteToBoardStrategy();
        }

        // ...
    }

    private Strategy createStartRollStrategy() {}
    private Strategy createWriteToBoardStrategy() {}
}

and the getAdvise will look like:

public class StrategyAdviser {

    private final YahtzeeGame yahtzeeGame;
    private final StrategyFactory strategyFactory;

    // constructor

    public Strategy predict() {
        return strategyFactory.build(yahtzeeGame);
    }
}

The benefit we now have is, that if you want to create different game rules, you can switch the strategyFactory.

For example: Currently a AmountOfIdenticals needs to be 5 for a special action. You could now create a custom StrategyFactory where the Amount needs to be 100 and if the rule is boring you can switch it back to the old strategyFactory without to touch the logic.

---

Flag Argument

A flag argument is a kind of function argument that tells the function to carry out a different operation depending on its value

YahtzeePlayer(String name, int color, boolean isHuman) {
   super(name, color, isHuman);
}

We do not need the flag.. We could just build two classes NonHumanPlayer and HumanPlayer and we can avoid the flag, which will build many branches in the code.

Answer 2

There are more than one way to play Yahtzee, so you need several strategies. Thus the StrategyFactory can not be instatiated in the StrategyAdviser, it has to be injected as a dependency.

The name suggests that StrategyAdviser would just be a proxy between the chosen Strategy and the ongoing game. The division of responsibilities should be clear and well documented.

At every state of the game, there are a known set of actions the player can take (mainly, 32 different combinations of roll/hold a dice in the hand). These actions should be enumerated by a component that is responsible for maintaining the rules of the game. The strategy should be provided with these actions as input and calculate the risk and reward for each one (related to the strategy type) as output. The role of the advisor would then be to implement riskiness (e.g. a risk averse or reckless advisor or maybe an advisor that just gives bad advise) and select one of the choices.