Bowling game scorer

I was given a simple coding exercise as part of a job interview process, for which I received a negative review.

This is the question:


DiUS is starting a bowling club. To help with the club, we have engaged you to program a scoring system.


The features on the system are:

• One player only

• In each frame, the bowler has 2 tries to knock down all the pins

• If in 2 tries, the bowler fails to knock down all the pins, their score is the sum of the number of pins they've knocked down in the 2 attempts

• E.g, if a bowler rolls, 4,4, then their score is 8.

-
If in 2 tries, the bowler knocks down all the pins, it is a spare. The scoring of a spare is the sum of the number of pins knocked down plus the number of pins knocked down in the next bowl.

• E.g, if a bowler rolls, 4,6 | 5,0, then their score is 20 = (4 + 6 + 5) + (5 + 0).

-
If in one try, the bowler knocks down all the pins, it is a strike. The scoring of a strike is the sum of the number of pins knocked down plus the number of pins knocked down in the next two bowls.

• E.g, if a bowler rolls, 10 | 5, 4, then their score is 28 = (10 + 5 + 4) + (5 + 4).

• There are 10 pins in a frame

• There are 10 frames in a match

• Don't worry about validating the number of rolls in a frame

The interface should look like this (in Java);

bowlingGame.roll(noOfPins);
bowlingGame.score();

Here is my solution:

MatchFactory.java

public final class MatchFactory {

    private MatchFactory(){};

    public static BowlingGame createMatch() {
        return new BowlingGameScoreBoard();
    }

}

BowlingGame.java

```
public interface BowlingGame {

/**
* Keeps track of pins knocked over
* @param noOfPins knocked over per frame
* @exception {@link au.com.dius.BowlingGameScoreBoard.BowlingException}
*/
public void roll(int noOfPins);

/**
*
* @return score of current frame only
*/

I agree with most of the feedback. There should be no roll after a strike in a frame. The frame contains only the strike roll.

It is usually a good idea to "hide" implementations of an interface with a factory, so that users of the interface don't depend on the concrete classes. But in this case, having a factory is too much since there is only one class implementing the interface and only one class using it (BowlingGameTest). You could even get rid of the interface.

The question explicitly states:


Don't worry about validating the number of rolls in a frame

The exercise assumes that calls to bowlingGame.roll(noOfPins) are always correct. So why bother with exceptions and checks like if (noOfPins > MAX_PINS)?

Having unit tests is good. :) Your test method names are quite close to describing what the test is about but they could be even better. Imagine folding all the test code and just reading the method names: one should be able to know what the tested code is supposed to do by reading just that. The names don't need to start with the word "test", we are in a test class after all and the @Test annotation already says it. Your tests are missing some simple cases: what happens if the player rolls 0 for the whole game? or all spares? (there is testStrikeEveryRoll() though)

More notes on test style:

int score = game.score();
Assert.assertEquals(300, score);

That score variable above is useless, inline it. There are too many empty lines for my taste, it makes me scroll to read everything thus degrades readability. You could also extract the loop that rolls several times in a separate method to make the tests more readable.

private void rollMany(int pins, int times) {
  for (int i = 0; i < times; i++) {
    game.roll(pins);
  }
}

Finally, the scoring rules in score() don't jump out in the reader's mind. Although this bit

if(prev.isSpare()) {
  score += (prev.score() + curr.getFirstScore());
}
if(prev.isStrike()) {
  score += (prev.score() + curr.getFirstScore() + curr.getSecondScore());
}

is starting to look like the real thing. For reference, here is how the score() method looks like in Robert Martin's Bowling Game Kata:

public int score() {
  int score = 0;
  int frameIndex = 0;
  for (int frame = 0; frame < 10; frame++) {
    if (isStrike(frameIndex)) {
      score += 10 + strikeBonus(frameIndex);
      frameIndex++;
    } else if (isSpare(frameIndex)) {
      score += 10 + spareBonus(frameIndex);
      frameIndex += 2;
    } else {
      score += sumOfBallsInFrame(frameIndex);
      frameIndex += 2;
    }
  }
  return score;
}

Aren't the rules obvious here? ;)

If you want to practice this exercise, I recommend http://cyber-dojo.org/. You can do group sessions and compare your solutions. I did it with my colleagues several times to train TDD, it was fun.

StackExchange Code Review Q#59560, answer score: 5