Java code for scoring bowling

Question

I've prepared a tenpin bowling scoring game in Java - https://github.com/chalcrow/DiusBowlingGame

I'd like a critical review of the code to identify potential improvements. The code runs like this:

Main.java

package com.dius.bowling;

public class Main {

    private DiusBowlingGame game;

    public static void main(String[] args) {
        // method to perform rolls and calculate totalscore
        DiusBowlingGame game = new DiusBowlingGame();
        // for a new game, we must advance to the 1st frame
        game.startGame();
        game.simulateGame();
    }
}

Main interface (BowlingGame.java)

package com.dius.bowling;

/**
 * Interface for a bowling game.
 */
public interface BowlingGame {
    /**
     * roll method specifying how many pins have been knocked down
     * @param noOfPins no. of pins knocked down
     */
    void roll(int noOfPins);

    /**
     * get player's current score
     * @return player's current score
     */
    int score();
}

DiusBowlingGame.java

package com.dius.bowling;
import java.util.*;

import static com.dius.bowling.Constants.*;

/**
 * Scoring system for tenpin bowls
 */
public class DiusBowlingGame implements BowlingGame {

    ArrayList<BowlingFrame> gameFrames = new ArrayList<BowlingFrame>();

    public void roll (int noOfPins) {

        System.out.println("frame" + getCurrentFrameNo() + ", rolled " + noOfPins);

        doSpecialScoresForPreviousFrame(noOfPins);

        getCurrentFrame().frameScore += noOfPins;

        if (isFirstRollOfFrame()) {
            getCurrentFrame().roll1 = new Roll();
            getCurrentFrame().roll1.pinsKnockedDown = noOfPins;
            // a strike can only occur on the 1st roll - we need to check if this is a strike
            getCurrentFrame().roll1.isStrike = this.isStrike(noOfPins);
            displayScore();
            if (this.isStrike(noOfPins)) {
                advanceFrame();
            }
        }
        else {
            // this is the 2nd roll
            getCurrentFrame().roll2 = new Roll();
            getCurrentFrame().roll2.pinsKnockedDown = noOfPins;
            // a spare can only occur on the 2nd roll - we need to check if this is a spare
            getCurrentFrame().roll2.isSpare = this.isSpare(noOfPins);
            displayScore();

            advanceFrame();
        }
    }

    private void doSpecialScoresForPreviousFrame(int noOfPins) {
        if (gameFrames.size() > 1) { // there has been at least one previous frame
            // check if any additional score should be added to the previous frame
            // for a strike on the previous frame, increment that frame score by score of both rolls on the current frame
            // for a spare on the previous frame, increment that frame score by score of the 1st roll on the current frame
            if(
                    getPreviousFrame().roll1.isStrike ||
                    getPreviousFrame().roll2.isSpare && isFirstRollOfFrame()
            ) {
                getPreviousFrame().frameScore += noOfPins;
            }
        }
    }

    public BowlingFrame getCurrentFrame() {
        BowlingFrame currentFrame = gameFrames.get(gameFrames.size() - 1);
        return currentFrame;
    }

    private int getCurrentFrameNo() {
        return gameFrames.size();
    }

    public BowlingFrame getPreviousFrame() {
        return gameFrames.get(gameFrames.size() - 2);
    }

    private boolean isFirstRollOfFrame() {
        return getCurrentFrame().roll1 == null;
    }

    private boolean isSpare(int noOfPins) {
        return getCurrentFrame().roll1.pinsKnockedDown + noOfPins == pinsPerFrame;
    }

    private boolean isStrike(int noOfPins) {
        return noOfPins == pinsPerFrame;
    }

    /**
     * Activate the 1st frame of the game
     */
    public void startGame() {
        advanceFrame();
    };

    public void advanceFrame() {
        if (gameFrames.size() < framesPerGame) {
            BowlingFrame newFrame = new BowlingFrame();
            gameFrames.add(newFrame);
        }
        else completeGame();
    }

    private void completeGame() {
        // TODO: do something to render the game complete
    }

    public int score() {
        int totalGameScore = 0;

        for (int i=0; i <= gameFrames.size() - 1; i++) {
            totalGameScore += gameFrames.get(i).frameScore;
        }

        return totalGameScore;
    }

    public void simulateGame() {
        //Frame 1- strike
        roll(10);
        //Frame2
        roll(3);
        roll(0);
        //Frame3 - wipeout
        roll(0);
        roll(0);
        //Frame 4 - spare
        roll(4);
        roll(6);
        //Frame 5
        roll(2);
        roll(6);
        //Frame 6 - spare
        roll(5);
        roll(5);
        //Frame 7 - strike
        roll(10);
        //Frame 8 - strike
        roll(10);
        //Frame 9 - spare
        roll(2);
        roll(8);
        //Frame 10 -spare
        roll(0);
        roll(10);
    }

    private void displayScore() {
        System.out.println("Total score - " + score());
    }
}

BowlingFrame.java

package com.dius.bowling;

public class BowlingFrame {

    public Roll roll1 = null;
    public Roll roll2 = null;
    public int frameScore = 0;

    public BowlingFrame() {}
}

Roll.java

package com.dius.bowling;

public class Roll {

    public int pinsKnockedDown;

    public boolean isStrike;

    public boolean isSpare;

    public Roll() { }
}

Answer 1

BowlingFrame and Roll contain unused information. frame.roll1.isSpare and frame.roll2.isStrike are never used. It would make more sense for isStrike and isSpare to be members of BowlingFrame, at which point Roll would only contain pinsKnockedDown, so that class should be removed, and BowlingFrame instead contain two integers for the pin counts for roll 1 & 2.

However, isStrike and isSpare are mutually exclusive, so perhaps an enum could be used to express STRIKE and SPARE states. You could also add states for UNPLAYED, PARTIAL & FINISHED to track whether or not there are rolls left in the frame.

The 10th frame scoring is flawed. A strike grants two additional rolls; a spare grants one additional roll. Neither of these are currently handled.

Answer 2

please find my initial observations below.

DiusBowlingGame: looks like a GOD class to me, you should try to reduce the responsibilities here. for example, Game simulation should never be part of the game itself and also, you can think of scoring logic as a separate strategy class on it own

Answer 3

As a completion of previous comments:

Main.java
- remove private variable and make local game variable final

DiusBowlingGame.java
- declare gameFrames as List<BowlingFrame> (this is why interfaces were invented :) )

BowlingFrame.java
- create getter and setter for each public field and make them private at declaration time (OOP - incapsulation)

Roll.java
- same as in BowlingFrame.java