Bowling game scorer

Question

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 */ public int score(); }

BowlingGameScoreBoard.java

public final class BowlingGameScoreBoard implements BowlingGame { private final List<Frame> frames; private static final int MAX_FRAMES = 10; private static final int MAX_PINS = 10; private static final int MAX_ATTEMPTS_PER_FRAME = 2; private int frameCounter = 0; private int strikeCounter = 0; private static final int ALL_STRIKE_SCORE = 300; /** * setup the game, ie all {@link BowlingGameScoreBoard#MAX_FRAMES} frames */ public BowlingGameScoreBoard() { frames = new ArrayList<Frame>(MAX_FRAMES); for (int i = 0; i < MAX_FRAMES; i++) { frames.add(new Frame()); } } @Override public void roll(int noOfPins) { if (noOfPins > MAX_PINS) { throw new BowlingException("illegal argument " + noOfPins); } Frame frame = getFrame(); if (frame == null) { throw new BowlingException("all attempts exhausted - start new game"); } frame.setScore(noOfPins); if (isBonusFrame()) { Frame prev = getPreviousFrame(); // restrict to one attempt, when last frame was spare if (prev.isSpare()) { frame.limitToOneAttempt(); } } } /** * returns a frame and moves to next frame if current has used up attempts * @return {@link au.com.dius.BowlingGameScoreBoard.Frame} */ private Frame getFrame(){ Frame frame = getCurrentFrame(); if (frame.isDone()) { // new bonus frame if(isLastFrame() && (frame.isSpare() || frame.isStrike())) { Frame bonus = new Frame(); frames.add(bonus); frameCounter++; return bonus; } frameCounter++; if (frameCounter == MAX_FRAMES || isBonusFrame()) { return null; } frame = getCurrentFrame(); } return frame; } @Override public int score() { int score; // first frame if (frameCounter == 0) { Frame curr = getCurrentFrame(); return curr.score(); } else { // score 300, strikes for all frames if (isLastFrame() && isAllStrikes()) { return ALL_STRIKE_SCORE; } Frame curr = getCurrentFrame(); Frame prev = getPreviousFrame(); // only add previous last frame to current score if (isBonusFrame()) { return prev.score() + curr.score(); } score = curr.score(); if(prev.isSpare()) { score += (prev.score() + curr.getFirstScore()); } if(prev.isStrike()) { score += (prev.score() + curr.getFirstScore() + curr.getSecondScore()); } } return score; } private Frame getPreviousFrame() { return frames.get(frameCounter-1); } private Frame getCurrentFrame() { return frames.get(frameCounter); } private boolean isAllStrikes() { return strikeCounter == MAX_FRAMES ; } private boolean isBonusFrame() { return frames.size() > MAX_FRAMES; } private boolean isLastFrame() { return frameCounter == MAX_FRAMES - 1; } /** * This nested class encapsulates the concept of a frame * and manages score and attempts allowed for each frame */ private class Frame { private int[] scores = new int[MAX_ATTEMPTS_PER_FRAME]; private int noOfPins = 10; private int noAttempts = 0; private boolean isStrike = false; private boolean isSpare() { return noOfPins == 0 && noAttempts == MAX_ATTEMPTS_PER_FRAME && !isStrike; } private boolean isStrike() { return noOfPins == 0 && noAttempts == MAX_ATTEMPTS_PER_FRAME && isStrike; } private boolean isDone () { return noAttempts == MAX_ATTEMPTS_PER_FRAME; } private void setScore(int score) { scores[noAttempts++] = score; noOfPins -= score; // keep track of remaining pins/frame if (score == MAX_PINS) { isStrike = true; strikeCounter++; } } private void limitToOneAttempt(){ scores[1] = 0; noAttempts++; } private int score() { return scores[0] + scores[1];} private int getFirstScore() { return scores[0]; } private int getSecondScore() { return scores[1]; } } /** * Represents an exception for the bowling game */ public class BowlingException extends RuntimeException { BowlingException(String message) { super(message); } } }

BowlingGameTest.java

@RunWith(JUnit4.class) public class BowlingGameTest { private BowlingGame game; private static final int MAX_ATTEMPTS = 20; @Before public void setUp() { game = MatchFactory.createMatch(); } @Test public void testScoreNoSpareOrStrike() { game.roll(4); game.roll(4); int score = game.score(); Assert.assertEquals(8, score); } @Test public void testSpare() { game.roll(4); game.roll(6); int score = game.score(); Assert.assertEquals(10, score); game.roll(5); game.roll(0); score = game.score(); Assert.assertEquals(20, score); } @Test public void testStrikeOnSecondAttempt() { game.roll(0); game.roll(10); int score = game.score(); Assert.assertEquals(10, score); game.roll(5); game.roll(4); score = game.score(); Assert.assertEquals(28, score); } @Test public void testStrikeOnFirstAttempt() { game.roll(10); game.roll(0); int score = game.score(); Assert.assertEquals(10, score); game.roll(5); game.roll(5); score = game.score(); Assert.assertEquals(30, score); } @Test public void testStrikeEveryRoll() { for (int i = 0; i < 10 ; i++) { game.roll(10); game.roll(0); } int score = game.score(); Assert.assertEquals(300, score); } @Test public void testLastFrameSpare() { for (int i = 0; i < 10 ; i++) { game.roll(5); game.roll(5); } game.roll(5); int score = game.score(); Assert.assertEquals(15, score); } @Test public void testLastFrameStrike() { for (int i = 0; i < 10 ; i++) { game.roll(10); game.roll(0); } game.roll(3); game.roll(4); int score = game.score(); Assert.assertEquals(17, score); } @Test(expected = BowlingGameScoreBoard.BowlingException.class) public void testLastFrameNoStrike() { for (int i = 0; i < 10 ; i++) { game.roll(3); game.roll(5); } // this wont happen as last frame wasnt strike/spare game.roll(3); game.roll(4); } /** * Exception is generated if try and go beyond 10 frames / match */ @Test(expected = BowlingGameScoreBoard.BowlingException.class) public void testPlayMoreThanAllFrames() { for (int i = 0; i <= MAX_ATTEMPTS ; i++) { game.roll(i/10); } } /** * This tests an illegal argument , ie cant pass more than 10 pins to * knock down * * I'am using custom exception here instead of Java's {@link java.lang.IllegalArgumentException} */ @Test(expected = BowlingGameScoreBoard.BowlingException.class) public void testIllegalBowlException() { game.roll(200); } }

The feedback I got was:

Pros

Has test

Broken up the solution into 2 classes (Bowling Game/Frame)

Cons

Doesn't roll again if it is a strike

MatchFactory seems unnecessary

Scoring method is a bit complicated

Doesn't handle multiple strike scenario properly

Not sure why the Frame is a private class

I'd like a review and agreements/disagreements with the feedback given.

Your solution seems over-engineered. Have a look at the bowling game kata for a much simpler way of doing it. — Kolargol00
– Kolargol00, Commented Aug 9, 2014 at 5:54
The rules are wrong. There is nothing about bonus balls in there. edit: that was optional, ok.although it is IMO harder not to implement that than to do. — user7610
– user7610, Commented Oct 11, 2015 at 9:34

Kolargol00 · Accepted Answer · 2014-08-09 06:46:01Z

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.

I must say that I cannot understand the code snippet of Robert Martin (without looking at the rest of code). What does ‘frameIndex‘ represent? — Emanuele Paolini
– Emanuele Paolini, Commented Aug 10, 2014 at 6:30
@EmanuelePaolini frameIndex is an index of the array containing all rolls for the game. It indicates the roll being currently processed. — Kolargol00
– Kolargol00, Commented Aug 11, 2014 at 7:23

200_success · Accepted Answer · 2014-08-12 15:11:11Z

Unit tests

Your unit tests contain significant inaccuracies. Unfortunately, a bug in a test casts doubt on both the test code and the main program.

@Test public void testStrikeOnSecondAttempt() { game.roll(0); game.roll(10); int score = game.score(); Assert.assertEquals(10, score); game.roll(5); game.roll(4); score = game.score(); Assert.assertEquals(28, score); }

Those rolls are actually a spare, followed by a frame of 5,4. The score should be \$(0 + 10 + \color{red}{5}) + (5 + 4) = 24\$, with the red 5 being a bonus for the spare.

Judging from the name of the test and the asserted result, I would guess that you probably meant to roll 0,0 | 10 | 5,4. Then, the score should be \$(0 + 0) + (10 + \color{red}{5 + 4}) + (5 + 4) = 28\$.

@Test public void testStrikeOnFirstAttempt() { game.roll(10); game.roll(0); int score = game.score(); Assert.assertEquals(10, score); game.roll(5); game.roll(5); score = game.score(); Assert.assertEquals(30, score); }

The rolls represent frames 10 | 0,5 | 5,?, where the third frame is incomplete. The score for those rolls should be \$(10 + \color{red}{0 + 5}) + (0 + 5) + 5 = 25\$, not 30.

Perhaps you meant to roll 10 | 5,5. The score for that would be \$(10 + \color{red}{5 + 5}) + (5 + 5 + \color{red}{?}) = 30\$, with the upcoming roll to be doubled as a bonus for the spare in the second frame.

@Test public void testStrikeEveryRoll() { for (int i = 0; i < 10 ; i++) { game.roll(10); game.roll(0); } int score = game.score(); Assert.assertEquals(300, score); }

A perfect game should consist of a dozen rolls of 10 (ten strikes, plus two rolls just for their bonuses). There shouldn't be any gutter balls. What you actually wrote was 10 | 0,10 | 0,10 | 0,10 | 0,10 | 0,10 | 0,10 | 0,10 | 0,10 | 0,10 | 0. The score for that unfinished would be 110, with the bowler unable to capitalize on any bonuses other than the third roll.

@Test public void testLastFrameSpare() { for (int i = 0; i < 10 ; i++) { game.roll(5); game.roll(5); } game.roll(5); int score = game.score(); Assert.assertEquals(15, score); }

That made no sense at all. From those rolls, just the pins alone should be valued at 105… and bonuses for the spares would add another 50. Those rolls have nothing to do with the name of the test, either.

A sequence that would satisfy both the assertion and the name of the test might be 0,0 | 0,0 | 0,0 | 0,0 | 0,0 | 0,0 | 0,0 | 0,0 | 0,0 | 0,10 | 5.

@Test public void testLastFrameStrike() { for (int i = 0; i < 10 ; i++) { game.roll(10); game.roll(0); } game.roll(3); game.roll(4); int score = game.score(); Assert.assertEquals(17, score); }

That is just as nonsensical as the previous test. The sequence 10 | 0,10 | 0,10 | 0,10 | 0,10 | 0,10 | 0,10 | 0,10 | 0,10 | 0,10 | 0,3 | 4 is illegal, as the game ended before rolling the 3.

A possible sequence that satisfies both the assertion and the name of the test might be 0,0 | 0,0 | 0,0 | 0,0 | 0,0 | 0,0 | 0,0 | 0,0 | 0,0 | 10 | 3,4.

@Test(expected = BowlingGameScoreBoard.BowlingException.class) public void testLastFrameNoStrike() { for (int i = 0; i < 10 ; i++) { game.roll(3); game.roll(5); } // this wont happen as last frame wasnt strike/spare game.roll(3); game.roll(4); }

This test is somewhat buggy, as it yields false confidence. After ten frames, each of 3,5, the game would have ended. Rolling 3 should be sufficient to trigger an exception. Therefore, writing your test to also roll 4 is being too lenient, by giving it a second chance to throw an exception that should have been thrown earlier.

@Test(expected = BowlingGameScoreBoard.BowlingException.class) public void testPlayMoreThanAllFrames() { for (int i = 0; i <= MAX_ATTEMPTS ; i++) { game.roll(i/10); } }

This is not a bug per se. However, the loop header is a bit misleading. Idiomatic ways to loop N times would be either

for (int i = 0; i < N; i++)

or

for (int i = 1; i <= N; i++)

Mixing the two idioms, therefore, gives a wrong impression about the number of times the loop will be executed. For clarity, I suggest

for (int i = 0; i < MAX_ATTEMPTS; i++) { game.roll(i/10); } // Game just ended. Exception thrown here... game.roll(2);

@Test(expected = BowlingGameScoreBoard.BowlingException.class) public void testIllegalBowlException() { game.roll(200); }

This is the only flawless test. Even then, 200 seems a bit excessive for an illegal value.

Conclusion about unit tests

Normally, mistakes in the unit tests might not be an immediate reason to disqualify a candidate. However, you got nearly every single one of them wrong, which seems to indicate either communication issues or consistent carelessness.

To me it is quite clear that in the code game.roll(10); game.roll(0); is for one strike. i.e. if a frame has a strike, it is treated in the code as a 10 followed by a 0. This is itself a bad thing though. You are correct though that many of the tests are still flawed. — Simon Forsberg
– Simon Forsberg, Commented Aug 12, 2014 at 16:59
@SimonAndréForsberg Perhaps the program wants to represent a strike internally as a frame of (10,0), but the interface is defined such that it should still be just one call: game.roll(10);. — 200_success
– 200_success, Commented Aug 12, 2014 at 17:03

Stack Exchange Network

Bowling game scorer

2 Answers 2

Unit tests

Conclusion about unit tests

You must log in to answer this question.

Hot Network Questions

Bowling game scorer

2 Answers 2

Unit tests

Conclusion about unit tests

You must log in to answer this question.

Related

Hot Network Questions