Bowling Game Kata

Question

I wanted to practice my programming skill and I started solving exercises from kata-logs site. The exercise:

The game consists of 10 frames. In each frame the player has two rolls to knock down 10 pins. The score for the frame is the total number of pins knocked down, plus bonuses for strikes and spares.

A spare is when the player knocks down all 10 pins in two rolls. The bonus for that frame is the number of pins knocked down by the next roll.

A strike is when the player knocks down all 10 pins on his first roll. The frame is then completed with a single roll. The bonus for that frame is the value of the next two rolls.

In the tenth frame a player who rolls a spare or strike is allowed to roll the extra balls to complete the frame. However no more than three balls can be rolled in tenth frame.

My solution:

class Frame {
    private Optional<Integer> firstRole = Optional.empty();
    private Optional<Integer> secondRole = Optional.empty();
    private Optional<Frame> next = Optional.empty();

    public void knock(int knockedPins) {
        if (secondRole.isPresent() && firstRole.isPresent()) {
            if (!next.isPresent()) {
                Frame nextFrame = new Frame();
                nextFrame.knock(knockedPins);
                next = Optional.of(nextFrame);
                return;
            }
            next.ifPresent(frame -> frame.knock(knockedPins));
            return;
        }

        if (firstRole.isPresent()) {
            secondRole = Optional.of(knockedPins);
            return;
        }

        if (knockedPins == 10) {
            firstRole = Optional.of(knockedPins);
            secondRole = Optional.of(0);
            next = Optional.of(new Frame());
            return;
        }
        firstRole = Optional.of(knockedPins);
    }

    public int finishedFramesCount() {
        if (!next.isPresent()) {
            if (frameFinished()) {
                return 1;
            }
            return 0;
        } else {
            return 1 + next.get().finishedFramesCount();
        }
    }

    public int points() {
        int knockedPins = firstRole.orElse(0) + secondRole.orElse(0);
        int bonusPointsForSpare = isSpare() ? next.map(Frame::firstRoleScore).orElse(0) : 0;
        int bonusPointsForStrike = isStrike() ? next.map(frame -> frame.firstRoleScore() + frame.secondRoleScore()).orElse(0) : 0;
        int totalResult = knockedPins + bonusPointsForStrike + bonusPointsForSpare;
        return next.map(frame -> totalResult + frame.points()).orElse(totalResult);
    }

    public Frame lastFrame() {
        if (!next.isPresent()) {
            return this;
        } else {
            return next.get().lastFrame();
        }
    }

    public boolean isSpare() {
        return secondRole.isPresent() && secondRole.get() > 0 && firstRole.get() + secondRole.get() == 10;
    }

    public boolean isStrike() {
        return firstRole.isPresent() && firstRole.get() == 10;
    }

    private boolean frameFinished() {
        return firstRole.isPresent() && secondRole.isPresent();
    }

    private int firstRoleScore() {
        return firstRole.orElse(0);
    }

    private int secondRoleScore() {
        return secondRole.orElse(0);
    }
}


class StandardBowlingGame implements BowlingGame {

    private final Frame frame;
    private int bonusRole;

    public StandardBowlingGame(Frame frame) {
        this.frame = frame;
    }

    @Override
    public void roll(int knockedPins) {
        if (frame.finishedFramesCount() == 10) {
            if (frame.lastFrame().isStrike() || frame.lastFrame().isSpare()) {
                bonusRole = knockedPins;
                return;
            }
            throw new IllegalStateException("Game is over");
        }
        frame.knock(knockedPins);
    }

    @Override
    public int score() {
        return frame.points() + bonusRole;
    }
}

Answer 1

I read the question and understood that BowlingGame interface is given. Regardless, your design is overly complex and breaks the SOLID Design Principles (which I understood are part of the purpuses).

1. The Frame class should be concerned about the logic of one frame and one frame only.
   instead, it is also the collection that holds all frames, and it also decides on the implementation of that collection - a user-defined linked list (re-inventing the wheel, since the JDK has a ready made implementation), the class is also responsible for deciding when and how to instantiate a new frame, and even when the game is finished (deciding if it is the last frame). All of these topics are outside of the responsibilities of a single Frame.
   Because of that, the Game class has indeed really very little to do besides delegete all logic to the frame.

2. Taking linked list as the collection seems unnecessary here, given that the number of frames is constant. in such cases (length is known in advance and it is a relatively small number) it would be better to use a plain array Frame[] game = new Frame[10]; that tells in one line how many frames are in one game. (instead of using recursion, really? for such a simple task use recursion??) and of course, declaring and manipulating the array is the responsibility of the Game class.
   ...and while we are at the point, there are (usually) two rolls per frame, so why not make that into an array as well instead of two separate variables? the advantages of using an array over two separate variables: a frame gets a new roll() and it assigns the new roll to the next empty place in the array (instead of asking on a seperate variable) and hence the last frame might be initialised with an array of length==3.

3. Regarding the use of Optional: it was introduced to Java 8 mainly as a solution to NullPointerException that gets thrown when the developer forget to explicitly check for this condition. in most cases, it occurs in method chaining such as map.get(key).toString(). since the exception is unchecked and thus gets thrown at run time, there was no way for the compiler to check this and it was a common error. so while it is feasible, its really isn't a good use case. I would go with a primitive int that is initialized to a negative value. to avoid magic numbers anti pattern you can define and use a constant public static final int EMPTY_ROLL = -1;