Calculate the score of a bowling game

Question

Before a job interview, I was asked to create a program that can calculate the score of a bowling game. The full assignment and my code can be viewed on my Github. The company told me that this was not good enough, so I would like to get feedback about what's wrong and how to improve it.

Assignment

The game consists of 10 frames as shown above. In each frame the player has two opportunities to knock down 10 pins. The score for the frame is the total number of pins knocked down, plus bonuses for strikes and spares.

A spare is when the player knocks down all 10 pins in two tries. The bonus for that frame is the number of pins knocked down by the next roll. So in frame 3 above, the score is 10 (the total number knocked down) plus a bonus of 5 (the number of pins knocked down on the next roll.)

A strike is when the player knocks down all 10 pins on his first try. The bonus for that frame is the value of the next two balls rolled.

In the tenth frame a player who rolls a spare or strike is allowed to roll the extra balls to complete the frame. However no more than three balls can be rolled in tenth frame.

Requirements

Write a class named “Game” that has two methods

roll(pins : int) is called each time the player rolls a ball. The argument is the number of pins knocked down.

score() : int is called only at the very end of the game. It returns the total score for that game.

My solution

Program.cs

using System;

namespace BowlingScore
{
    class Program
    {
        static void Main(string[] args)
        {
            var game = new Game();

            game.Roll(1);
            game.Roll(4);

            game.Roll(4);
            game.Roll(5);

            game.Roll(6);
            game.Roll(4);

            game.Roll(5);
            game.Roll(5);

            game.Roll(10);

            game.Roll(0);
            game.Roll(1);

            game.Roll(7);
            game.Roll(3);

            game.Roll(6);
            game.Roll(4);

            game.Roll(10);

            game.Roll(2);
            game.Roll(8);
            game.Roll(6);

            Console.WriteLine("Score: " + game.Score());
            Console.ReadLine();
        }
    }
}

Game.cs

using System;
using System.Collections.Generic;
using System.Linq;

namespace BowlingScore
{
    internal class Game
    {
        /// <summary>
        /// Maximum number of Frames allowed in this game
        /// </summary>
        private const int MaxFrameCount = 10;

        /// <summary>
        /// Number of pins that each Frame should start with
        /// </summary>
        private const int StartingPinCount = 10;

        private readonly List<Frame> frames = new List<Frame>();
        private int score;

        /// <summary>
        /// Register the result of a roll
        /// </summary>
        /// <param name="knockedDownPins">How many pins have been knocked down</param>
        public void Roll(int knockedDownPins)
        {
            if (frames.Count == MaxFrameCount && frames.Last().IsClosed)
            {
                throw new InvalidOperationException("You've played enough for today! Consider calling Score()");
            }

            if (!frames.Any() || frames.Last().IsClosed)
            {
                var isLastFrame = frames.Count == MaxFrameCount - 1;
                frames.Add(new Frame(StartingPinCount, isLastFrame));                
            }

            frames.Last().RegisterRoll(knockedDownPins);
        }

        /// <summary>
        /// Get the total score
        /// </summary>
        /// <returns>The total score calculated from all Frames</returns>
        public int Score()
        {
            for (var frameIndex = 0; frameIndex < frames.Count; frameIndex++)
            {
                var frame = frames[frameIndex];
                var frameScore = 0;
                var bonusScore = 0;
                var isStrike = false;

                // cap the roll index to 2 to avoid over-counting points if the last frame has bonus rolls
                var maxRollIndex = frame.RollResults.Count < 2 ? frame.RollResults.Count : 2;

                for (var rollIndex = 0; rollIndex < maxRollIndex; rollIndex++)
                {
                    var result = frame.RollResults[rollIndex];
                    frameScore += result;

                    // calculate bonus score for a strike
                    if (result == StartingPinCount)
                    {
                        isStrike = true;

                        // look 2 rolls ahead
                        bonusScore += CalculateBonusScore(frameIndex, rollIndex, 2);
                        break;
                    }
                }

                // calculate bonus score for a spare
                if (!isStrike && frameScore == StartingPinCount)
                {
                    // look 1 roll ahead
                    bonusScore += CalculateBonusScore(frameIndex, maxRollIndex - 1, 1);
                }

                score += frameScore + bonusScore;
            }

            return score;
        }

        /// <summary>
        /// Recursive function to calculate the bonus score of the next X rolls
        /// </summary>
        /// <param name="frameIndex">Index of the current frame</param>
        /// <param name="rollIndex">Index of the current roll</param>
        /// <param name="rollCount">How many rolls to look ahead</param>
        /// <returns>The amount of bonus score calculated from the next X rolls</returns>
        private int CalculateBonusScore(int frameIndex, int rollIndex, int rollCount)
        {
            if (rollCount == 0)
            {
                return 0;
            }

            var bonusPoints = 0;

            // add the next roll in the same frame, if any
            if (frames[frameIndex].RollResults.Count > rollIndex + 1)
            {
                bonusPoints += frames[frameIndex].RollResults[rollIndex + 1];
                bonusPoints += CalculateBonusScore(frameIndex, rollIndex + 1, rollCount - 1);
            }
            else
            {
                // add the first roll of the next frame, if any
                if (frames.Count > frameIndex + 1)
                {
                    bonusPoints += frames[frameIndex + 1].RollResults[0];
                    bonusPoints += CalculateBonusScore(frameIndex + 1, 0, rollCount - 1);
                }
            }

            return bonusPoints;
        }
    }
}

Frame.cs

using System;
using System.Collections.Generic;

namespace BowlingScore
{
    internal class Frame
    {
        /// <summary>
        /// How many pins have been knocked down in each roll
        /// </summary>
        public List<int> RollResults { get; } = new List<int>();

        /// <summary>
        /// No more rolls can be registered on a closed Frame
        /// </summary>
        public bool IsClosed => !isLastFrame && standingPins == 0 || 
                                !isLastFrame && RollResults.Count == 2 ||
                                RollResults.Count == 3;

        private int standingPins;
        private readonly int startingPinCount;
        private readonly bool isLastFrame;
        private bool extraRollAllowed;

        /// <summary>
        /// Create a new Frame
        /// </summary>
        /// <param name="startingPinCount">Number of pins that the Frame should start with</param>
        /// <param name="isLastFrame">Special rules apply on the last frame</param>
        public Frame(int startingPinCount, bool isLastFrame = false)
        {
            this.startingPinCount = startingPinCount;
            standingPins = startingPinCount;
            this.isLastFrame = isLastFrame;
        }

        /// <summary>
        /// Register the result of a roll
        /// </summary>
        /// <param name="knockedDownPins">How many pins have been knocked down</param>
        public void RegisterRoll(int knockedDownPins)
        {
            ValidateRoll(knockedDownPins);
            RollResults.Add(knockedDownPins);
            standingPins -= knockedDownPins;
            ResetPinsIfNecessary();
        }

        private void ResetPinsIfNecessary()
        {
            if (isLastFrame && standingPins == 0)
            {
                standingPins = startingPinCount;
                extraRollAllowed = true;
            }
        }

        private void ValidateRoll(int knockedDownPins)
        {
            if (standingPins == 0)
            {
                throw new InvalidOperationException("Can't roll when there are no standing pins");
            }

            if (!isLastFrame && RollResults.Count == 2 || 
                isLastFrame && RollResults.Count == 2 && !extraRollAllowed ||
                RollResults.Count > 2)
            {
                throw new InvalidOperationException($"Can't register more than {RollResults.Count} rolls in this frame");
            }

            if (knockedDownPins < 0 || knockedDownPins > standingPins)
            {
                throw new InvalidOperationException($"Can't knock down {knockedDownPins} while there are only {standingPins} standing pins");
            }
        }
    }
}

Answer 1

The company told me that this was not good enough, so I would like to get feedback about what's wrong and how to improve it.

Fundamentally, you have to ask them. We can give your our thoughts, but it might come down to company culture / style.

---

    class Program
    {
        static void Main(string[] args)
        {
            var game = new Game();

            game.Roll(1);
            ...
            game.Roll(6);

            Console.WriteLine("Score: " + game.Score());

To be honest, I'd prefer to submit the class with no tests whatsoever than with this "test". It's very long-winded (why not use a loop for the rolls?) and doesn't even compare the observed score with the expected score.

Better, though, would be to provide a test suite using a proper test framework (NUnit, Microsoft.VisualStudio.TestTools.UnitTesting, ...) with a series of tests to cover the corner cases and comments to show what corner cases they cover: at least the exceptions, scenarios with and without an extra roll in the last frame, and the maximum score.

---

My personal opinion is that the code is very complicated for the task it does. The scoring could be implemented without a separate method to calculate the bonus and with a single loop: it just needs variables to indicate whether the previous ball was a normal throw, a spare, or a strike; and whether the ball before that was a strike or not. Then on the basis of those variables you multiply the score for the current ball by 1, 2, or 3. If the person assessing the code was an architecture astronaut you're probably fine; if they were an optimisation obsessive then this might count quite heavily in their assessment.

---

        public List<int> RollResults { get; } = new List<int>();

It's a read-only property which can be freely modified. Prefer to expose IReadOnlyList<T> so that all modification has to be done by calling methods on the class.

---

            if (!isLastFrame && RollResults.Count == 2 || 
                isLastFrame && RollResults.Count == 2 && !extraRollAllowed ||
                RollResults.Count > 2)
            {
                throw new InvalidOperationException($"Can't register more than {RollResults.Count} rolls in this frame");
            }

DRY. The condition should be if (IsClosed) - and if it isn't, I suspect a bug.

---

There are other things I would do differently, but that's questions of style and shouldn't matter in an interview unless you've been given a style document to follow. The first two points are my best guess as to why the company rejected the code. The other two are minor points - they might count against you in comparison against another candidate, but I wouldn't expect them to disqualify you.

Answer 2

The code is enormously bloated and complicated for what it does. The score of each frame can be calculated in a simple loop, nicely encapsulated in a static method or extension method.

public static IEnumerable<int> Scores(this IList<int> pins)
{
    // Walk the list in steps of two rolls (= one frame)
    for (int i = 0; i + 1 < pins.Count; i += 2)
    {
        // Neither strike nor spare
        if (pins[i] + pins[i + 1] < 10)
        {
            yield return pins[i] + pins[i + 1];
            continue;
        }

        // Score can only be determined if third roll is available
        if (i + 2 >= pins.Count)
            yield break;

        yield return pins[i] + pins[i + 1] + pins[i + 2];

        // In case of strike, advance only by one
        if (pins[i] == 10)
            i--;
    }
}

The required interface can then be implemented in a small wrapper class.

public class Bowling
{
    private List<int> pins = new List<int>();

    public void Roll(int n) => pins.Add(n);

    public int Score() => pins.Scores().Take(10).Sum();
}