3
\$\begingroup\$

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();
    }

}
\$\endgroup\$
  • \$\begingroup\$ maybe i'm not 100% accurate with the official Rules but i don't think that really matters here.... \$\endgroup\$ – Martin Frank May 20 at 10:30
  • 2
    \$\begingroup\$ This is probably just a language barrier issue, but I've heard native speakers do it also, so: one die, multiple dice. There's no such thing as "dices". :) \$\endgroup\$ – Eric Stein May 20 at 15:48
  • \$\begingroup\$ oh my - yes, one dice, two die! thank you very much.... i'm definitly no native english speaker =).... i'll adjust my code! to keep consitency i will not alter my question here... \$\endgroup\$ – Martin Frank May 20 at 17:01
  • 1
    \$\begingroup\$ As far as your actual problem goes, have you considered using a chain of responsibility? I think it might require some rework of your current design outside of the StrategyAdvisor class, but it's something to consider. \$\endgroup\$ – Eric Stein May 20 at 17:03
  • 2
    \$\begingroup\$ I suggest that you include the Strategy and StrategyFactory classes as well, as they are likely part of the problem. \$\endgroup\$ – 200_success May 20 at 19:12
1
\$\begingroup\$

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

enter image description here

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.

\$\endgroup\$
  • \$\begingroup\$ Hello Roman, glad Rio see you back again. Again you pointed out much of basic mistakes which should be removed first. Actually I'm on holidays and can't respond quickly nor with a real device (currently I'm on the Mobil phone). Your review was very helpful so far. \$\endgroup\$ – Martin Frank Jun 11 at 8:24
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks Torben - my idea was to make the strategic advisor decide if it's a 'risk-ful' advisor or if it is a 'defensive' advisor - right now i am not there, so there is just one strategic advisor who has just this (see above) algorithm... The Strategy is used on the current roll - not on the whole game at all... i am still only online via mobile and can't yet provide more code - but i'll stick wiht it!! (+1) \$\endgroup\$ – Martin Frank May 21 at 16:53
  • \$\begingroup\$ ok, code is updated.... \$\endgroup\$ – Martin Frank May 21 at 18:40
  • \$\begingroup\$ uh - about my comment above - i hope that my updated question makes it more clear: the strategy appiels to the current Roll! (ok, i might rename it into RollingStrategy to prevent any further questions) it should just tell the AiPlayer what would be a good idea to do with the current roll. \$\endgroup\$ – Martin Frank May 21 at 19:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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