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
MatchFactoryseems unnecessary- Scoring method is a bit complicated
- Doesn't handle multiple strike scenario properly
- Not sure why the
Frameis a private class
I'd like a review and agreements/disagreements with the feedback given.
