3
\$\begingroup\$

I am sharing my Bowling Game Score Kata made in Java, it is available for whoever wants to code review it. I appreciate any comments.

class BowlingGameScore {

    private int[] pinsKnockedDown = new int[21];
    private int currentRoll = 0;

    void roll(int pinsKnockedDown) {
        this.pinsKnockedDown[currentRoll++] = pinsKnockedDown;
    }

    int score() {
        int totalScore = 0;
        int rollNumber = 0;

        for (int frameNumber = 0; frameNumber < 10; frameNumber++) {
            int frameScore;

            if (isSpare(rollNumber)) {
                frameScore = 10 + getSpareBonus(rollNumber);
                rollNumber += 2;

            } else if (isStrike(pinsKnockedDown[rollNumber])) {
                frameScore = 10 + getStrikeBonus(rollNumber);
                rollNumber++;

            } else {
                frameScore = pinsKnockedDown[rollNumber] + pinsKnockedDown[rollNumber + 1];
                rollNumber += 2;
            }

            totalScore += frameScore;
        }

        return totalScore;
    }

    private int getStrikeBonus(int roll) {
        return pinsKnockedDown[roll + 1] + pinsKnockedDown[roll + 2];
    }

    private boolean isStrike(int roll) {
        return roll == 10;
    }

    private int getSpareBonus(int roll) {
        return pinsKnockedDown[roll + 2];
    }

    private boolean isSpare(int roll) {
        return pinsKnockedDown[roll] + pinsKnockedDown[roll + 1] == 10;
    }
}

And its tests:

class BowlingGameScoreTest {
    private BowlingGameScore game;

    @BeforeEach
    void setUp() {
        game = new BowlingGameScore();
    }

    @Test
    void
    game_no_pins_get_hit() {
        rollMany(0, 20);

        Assertions.assertEquals(0, game.score());
    }

    @Test
    void
    game_just_1_pin_get_hit() {
        rollMany(1, 20);

        Assertions.assertEquals(20, game.score());
    }

    @Test
    void
    game_with_1_spare() {
        rollSpare();
        game.roll(3);
        rollMany(0, 17);

        Assertions.assertEquals(16, game.score());
    }

    @Test
    void
    game_with_1_strike() {
        game.roll(10);
        game.roll(3);
        game.roll(4);
        rollMany(0, 16);

        Assertions.assertEquals(24, game.score());
    }

    @Test
    void
    game_of_strikes() {
        rollMany(10, 12);

        Assertions.assertEquals(300, game.score());
    }

    @Test
    void
    knockdown_nine_and_miss_ten_times() {

        int firstRollPinsKnockedDown = 9;
        int secondRollPinsKnockedDown = 0;
        int repeatTimes = 10;
        rollPair(repeatTimes, firstRollPinsKnockedDown, secondRollPinsKnockedDown);

        Assertions.assertEquals(90, game.score());
    }


    @Test
    void
    knockdown_five_and_spare_ten_times_and_fice() {
        rollPair(10, 5, 5);
        game.roll(5);

        Assertions.assertEquals(150, game.score());
    }

    private void rollPair(int repeatTimes, int firstRollPinsKnockedDown, int secondRollPinsKnockedDown) {
        for (int i = 0; i < repeatTimes; i++) {
            game.roll(firstRollPinsKnockedDown);
            game.roll(secondRollPinsKnockedDown);
        }
    }

    private void rollSpare() {
        game.roll(5);
        game.roll(5);
    }


    private void rollMany(int pins, int times) {
        for (int i = 0; i < times; i++) {
            game.roll(pins);
        }
    }
}
\$\endgroup\$
1
  • 3
    \$\begingroup\$ The problem statement would be very helpful. \$\endgroup\$
    – vnp
    Jul 18, 2019 at 0:33

2 Answers 2

2
\$\begingroup\$

As far as BowlingGameScore goes, LGTM. I have no remarks. (And usually I have lots.)

In the tests, after each void (e.g. before game_no_pins_get_hit), it's a bit odd to have a newline.

Also, a static import to turn Assertions.assertEquals into a concise assertEquals would be convenient.

\$\endgroup\$
1
  • \$\begingroup\$ thank you for your code-review @J_H. I included the static import in my code. regarding the new lines after void, it is Intellij auto format, do you know how to customize it to keep the following format ? @ Test void (new line) game_no_pins_get_hit() { \$\endgroup\$ Jul 20, 2019 at 22:38
1
\$\begingroup\$

Kata compliance

Your solution is very close to the template. Bob seems to expect you to more or less type in his solution, working through the steps in order to end up with the same code, thus forming a way of thinking through repetition. Your code is slightly different, but still satisfies the requirements, and of course you may have worked from a different description. You've generated similar refactorings making the code easy to read.

The invisible bug

You have a slight bug in your code. I say slight, because it currently has no observable impact. Given three rolls (10, 0, 1). There's a strike, and a frame of 1, which would score strike (10) + bonus (0 + 1) + second frame (1) = 12. The way your code processes, it's a spare (10 + 0) + bonus (1) + second frame (1) = 12. The end result is the same, but it's running through your code in an unexpected route. This seems like something that 'could' cause future issues, if for example you wanted to provide a summary of the frame scores. It can be fixed by checking if it's a strike, before checking if it's a spare.

Test Formatting

Your tests are in a slightly weird format. I'd expect the layout to be:

@Test
void game_just_1_pin_get_hit() {

Consistency

In your tests you've got two methods that do a similar thing:

private void rollMany(int pins, int times) {
private void rollPair(int repeatTimes, int firstRollPinsKnockedDown, int secondRollPinsKnockedDown) {

One takes the number of times to perform the iteration as the second parameter, the other takes it as a first. This can be a bit misleading when reading the tests. Your initial test names start with game_of, setting the context that the test is checking an entire game. You then drop the prefix and instead go with knockdown, even though you're still testing the whole game in these tests. Taking a consistent naming approach can make it easier to know what to expect from the test.

Constants

The Kata template I've looked at has a similar approach to yours, so this may be more of a comment on the Kata, however I think the code could benefit from the use of some constants. This would help to set context for people that don't know the game, about the number of pins per frame and the maximum number of rolls in a standard game.

Error checking

Again, this seems to be more a reflection of the Kata itself, there's no error checking in the written code. There's nothing to check that you're not knocking down more than the number of pins the game allows, no checking that you're not trying to roll more balls than the class has capacity to deal with, etc. It's an important next step to consider what situations the class might need to handle.

Typo

There seems to be a typo in this test name:

knockdown_five_and_spare_ten_times_and_fice

Should be five not fice?

\$\endgroup\$

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.