1
\$\begingroup\$

This is my first bowling kata solution in java. I would be very grateful if you say something about that. I want to learn from you to become a better programmer. I think the score method is quite small, but is it clear enough as well? How can I improve it more?

public class BowlingGame {

  int[] rolls = new int[21];
  int currentRollIndex = 0;

  public void roll(int roll) {
      if (currentRollIndex < 21) {
          rolls[currentRollIndex++] = roll;
      }
  }

  public int score() {
      int score = 0;
      int index = 0;
      for (int i = 0; i < 10; i++) {
          score += rollIndex(index++);
          if (isStrike(index - 1)) score += rollIndex(index, index + 1);
           else {
              score += rollIndex(index++);
              if (isSpare(index - 2)) score += rollIndex(index);
          }
      }
      return score;
  }

  private boolean isStrike(int index) {
      return rolls[index] == 10;
  }

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

  private int rollIndex(int... indexes) {
      int sum = 0;
      for (int i = 0; i < indexes.length; i++) sum += rolls[indexes[i]];
      return sum;
  }
  }

JUnit test:

public class BowlingKataTest {

    public BowlingKataTest() {
    }
    Bowling bowling;

    @BeforeClass
    public static void setUpClass() {
    }

    @AfterClass
    public static void tearDownClass() {
    }

    @Before
    public void setUp() {
        bowling = new Bowling();
    }

    @After
    public void tearDown() {
    }

    @Test
    public void NullTest() {
        bowling.roll(0);
        assertEquals(0, bowling.score());
    }

    void setRolls(int n, int pin) {
        for (int i = 0; i < n; i++) {
            bowling.roll(pin);
        }
    }

    void set2Rolls(int n, int pin1, int pin2) {
        for (int i = 0; i < n; i++) {
            bowling.roll(pin1);
            bowling.roll(pin2);
        }
    }

    void setArrayRolls(int[] n) {
        for (int i = 0; i < n.length; i++) {
            bowling.roll(n[i]);

        }
    }

    @Test
    public void OnesTest() {
        setRolls(20, 1);
        assertEquals(20, bowling.score());
    }

    @Test
    public void OnesBonusTest() {
        setRolls(10, 1);
        bowling.roll(10);
        setRolls(8, 1);
        assertEquals(30, bowling.score());
    }

    @Test
    public void FullBonusTest() {
        setRolls(12, 10);
        assertEquals(300, bowling.score());
    }

    @Test
    public void FullHalfBonusTest1() {
        set2Rolls(10, 3, 7);
        bowling.roll(10);
        assertEquals(137, bowling.score());
    }
    @Test
    public void FullHalfBonusTest2() {
        setRolls(21, 5);
        assertEquals(150, bowling.score());
    }
    @Test
    public void RealTest() {
        int[] rolls = {0,10,5,5,3,2,10,10,3,7,7,1,4,2,9,1,8,2,5};
        setArrayRolls(rolls);
        assertEquals(140, bowling.score());
    }
}
\$\endgroup\$
0
\$\begingroup\$

Overall, looks good design. good breakdown into methods. Here are my comments:

  1. Avoid magic numbers
    Magic numbers are literals (String or Numeric values). They should be replced with constant variables (public static final ... and UPPER_CASE named). This has two benefits: 1) give meaning to the literal value (helps readability) and 2) one place to change value and also avoid (runtime!) errors due to incorrect value due to typo.
    Example: public static final int MAX_ROLL_COUNT = 21;

  2. Meaningful variable names This is particularly useful in your case where you have multiple index variables. You have both i and index. IIUC, i is index on frames (==turns in bowling jargon, and don't forget to replace literal used as loop limit with constant!) and index is index on rolls.

  3. Meaningful method names
    rollIndex() is not a good name. perhaps calculateScoreForFrame() or something similar

  4. Use collection streams
    Depending on your proficiency in the language, you may want to experiment with replace these for loops with Java 8 collection streams. It (arguably) makes the code even more clearer.

\$\endgroup\$
  • \$\begingroup\$ Thank You for your comment. Just one more question. \$\endgroup\$ – MAttti Oct 7 '18 at 17:43
  • \$\begingroup\$ Thank You for your comment. Just one more question. The variable 'i' of the 'for' loop in the 'score' method ('for (int i = 0; i < 10; i++) {') has no other function just iterate the loop 10 times. Should I use meaningful variable name in this case as well? \$\endgroup\$ – MAttti Oct 7 '18 at 18:20
  • \$\begingroup\$ the function iterate 10 times because there are 10 turns of the game. \$\endgroup\$ – Sharon Ben Asher Oct 8 '18 at 6:13
  • \$\begingroup\$ It is OK! But the loop variable (i) has no important meaning. (Only count to 10 ) Do I avoid the one letter variables in this case too? \$\endgroup\$ – MAttti Oct 8 '18 at 16:27
  • \$\begingroup\$ you are right, the one letter convention for loops is indeed an industry standard so it is better to leave it as is \$\endgroup\$ – Sharon Ben Asher Oct 9 '18 at 6:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.