5
\$\begingroup\$

I recently interviewed for a software engineering job that requires at least one year of C# and experience with the Microsoft stack. Even though I made it clear at the outset that I had zero experience with C#, they still wanted me to take their engineering test, which led me to believe that they were willing to give me a shot because I am really good at learning new programming languages and am otherwise a solid developer. Here are the details for the "bowling exercise" test they gave me:

To complete this coding exercise, you will need to write a class that implements the interface below and provides logic to score a bowling game according to the bowling scoring rules. If you are not familiar with the rules of bowling, we encourage you to briefly research the intricacies of the scoring rules. You can write your implementation in either C# or VB.NET.

You do not need to handle invalid input in your class. Your code does not need to have a user interface.


C# Interface

public interface ISimpleBowlingGame 
{
  // Called when a player completes a frame.
  // This method will be called 10 times for a bowling game.
  // The throws parameter provides the number of pins knocked down on each throw in the frame being recorded.
  // The 10th frame may have 3 values.
  void RecordFrame(params int[] throws);

  // Called at the end of the game to get the final score.
  int Score { get; }
}

That's it. That is all of the instruction they provided for this exercise.

So I worked on it for three nights and a whole weekend, and in the end thought I had nailed it. I learned how to implement an interface, set up classes, developed the logic, and included test cases that I thought proved that I had succeeded with this. All of the scoring tests I ran produced the correct final scores.

So I turned in the following code:

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

namespace bowlingScore
{

    public interface ISimpleBowlingGame
    {
        // Called when a player completes a frame.
        // This method will be called 10 times for a bowling game.
        // The throws parameter provides the number of pins knocked down on each throw in the frame being recorded.
        // The 10th frame may have 3 values.
        void RecordFrame(params int[] throws);

        // Called at the end of the game to get the final score.
        int Score { get; }
    }

    public class BowlingGame : ISimpleBowlingGame
    {
        public int Score { get; set; }
        public int frame = 0;           // The current frame
        public int frameScore = 0;      // The total of first two throws in a normal frame
        public int[] throws;            // Input
        public int throw1;              // First throw in a frame
        public int throw2;              // Second throw in a frame
        public int throw3;              // Third throw in the 10th frame

        public List<int> tenFrameScores = new List<int>();
        public int[][] tenFrames = new int[10][]; // a JaggedArray (to allow for three numbers in the 10th frame)

        // RecordFrame records all frames to a data container, to allow for easy scoring
        public void RecordFrame(params int[] throws)
        {
            throw1 = throws[0];
            // In case there is no second number (0) provided when someone throws a strike
            throw2 = (throws.Length == 1) ? 0 : throws[1];
            throw3 = (throws.Length == 3) ? throws[2] : 0;
            frameScore = throw1 + throw2;
            // Store the frame scores in a list as they come in, to be updated with strike and spare bonuses
            tenFrameScores.Add(frameScore);

            // Dynamically populate the JaggedArray tenFrames
            if (throws.Length == 3)
            {
                tenFrames[frame] = new[] { throw1, throw2, throw3 };
            } 
            else
            {
                tenFrames[frame] = new[] { throw1, throw2 };
            }

            Console.WriteLine("Throws: " + string.Join(",", throws));

            // Get the number of non-null elements in the tenFrames array
            int numberFramesSoFar = tenFrames.Count(s => s != null);
            //Console.WriteLine("Number of frames so far: " + numberFramesSoFar);
            frame++;

            if (numberFramesSoFar == 10)    // Ready to calculate a score
            {
                Console.WriteLine("Ten frames of data have been received. Ready to score the game.");
                for (int i = 0; i < 10; i++) // iterate through the data container of throws
                {

                    Console.WriteLine("tenFrames[" + i + "]: " + string.Join(",", tenFrames[i]));
                    throw1 = tenFrames[i][0];
                    throw2 = tenFrames[i][1];
                    frameScore = tenFrameScores[i];
                    //Console.WriteLine("Frame total: " + tenFrameScores[i]);


                    // Strikes and Spares in frames 1-9
                    if (frameScore == 10 && i < 9)
                    {
                        int nextFrameScore = tenFrameScores[i+1];

                        if (throw1 == 10)   // it's a strike
                        {
                            ColorMessage(ConsoleColor.Green, "STRIKE!");

                            // Check to see if the next throw was also a strike
                            if (tenFrames[i+1][0] == 10)   // it's a strike
                            {
                                // bonus = 10 + the first throw of the next frame
                                if (i < 8)
                                {
                                    int strikeBonus = 10 + tenFrames[i+2][0];
                                    Console.WriteLine("\tNext frame is a strike strikeBonus: " + strikeBonus);
                                    tenFrameScores[i] += strikeBonus;
                                }
                                else  // frame 9, look at just the first two throws of frame 10
                                {
                                    int strikeBonus = tenFrames[i+1][0] + tenFrames[i+1][1];
                                    Console.WriteLine("\tNext frame is a strike strikeBonus: " + strikeBonus);
                                    tenFrameScores[i] += strikeBonus;
                                }
                            }
                            else
                            {
                                // bonus = the total of the two throws in the next frame (nextFrameScore)
                                Console.WriteLine("\tstrikeBonus: " + nextFrameScore);
                                tenFrameScores[i] += nextFrameScore;
                            }
                        }
                        else // spare
                        {
                            ColorMessage(ConsoleColor.Cyan, "SPARE!");
                            Console.WriteLine("\tspareBonus: " + tenFrames[i+1][0]);
                            tenFrameScores[i] += tenFrames[i+1][0];
                        }
                    }
                    else    // Tenth frame bonuses
                    {
                        if (throw1 == 10) // strike in the tenth frame
                        {
                            ColorMessage(ConsoleColor.Green, "STRIKE! (in the 10th)");
                            // bonus = throw2 + throw3 (throw2 is already included in tenFrameScores[i])
                            Console.WriteLine("\tstrikeBonus: " + (throw2 + throw3));
                            tenFrameScores[i] += throw3;
                        }
                        else if (frameScore == 10) // spare in the first two throws of the 10th frame
                        {
                            ColorMessage(ConsoleColor.Cyan, "SPARE (in the 10th)!");
                            // bonus = last throw of the 10th frame
                            Console.WriteLine("\tspareBonus: " + throw3);
                            tenFrameScores[i] += throw3;
                        }
                    }
                }

                for (int i = 0; i < 10; i++)
                {
                    Score += tenFrameScores[i];
                    Console.WriteLine(tenFrameScores[i]);
                }
            }
        }

        public void ColorMessage(ConsoleColor color, string message)
        {
            // Set text color
            Console.ForegroundColor = color;

            // Write message
            Console.WriteLine(message);

            // Reset text color
            Console.ResetColor();
        }


    }
    public class Test
    {
        public static void Main()
        {
            BowlingGame newGame = new BowlingGame();

            // Test first game - SUCCESS! (190)
            //newGame.RecordFrame(4, 6);
            //newGame.RecordFrame(3, 6);
            //newGame.RecordFrame(2, 1);
            //newGame.RecordFrame(10); // 30
            //newGame.RecordFrame(10); // 30
            //newGame.RecordFrame(10); // 21
            //newGame.RecordFrame(10); // 20
            //newGame.RecordFrame(1, 9);
            //newGame.RecordFrame(10,0);
            //newGame.RecordFrame(10, 4, 6);

            // Test perfect game - SUCCESS! (300)
            //newGame.RecordFrame(10);
            //newGame.RecordFrame(10, 0);
            //newGame.RecordFrame(10);
            //newGame.RecordFrame(10);
            //newGame.RecordFrame(10);
            //newGame.RecordFrame(10);
            //newGame.RecordFrame(10);
            //newGame.RecordFrame(10);
            //newGame.RecordFrame(10);
            //newGame.RecordFrame(10, 10, 10);

            // Test all zeroes
            //newGame.RecordFrame(0, 0);
            //newGame.RecordFrame(0, 0);
            //newGame.RecordFrame(0, 0);
            //newGame.RecordFrame(0, 0);
            //newGame.RecordFrame(0, 0);
            //newGame.RecordFrame(0, 0);
            //newGame.RecordFrame(0, 0);
            //newGame.RecordFrame(0, 0);
            //newGame.RecordFrame(0, 0);
            //newGame.RecordFrame(0, 0);

            // Test successful (111)
            //newGame.RecordFrame(10, 0);
            //newGame.RecordFrame(2, 8);
            //newGame.RecordFrame(4, 3);
            //newGame.RecordFrame(0, 8);
            //newGame.RecordFrame(0, 5);
            //newGame.RecordFrame(7, 0);
            //newGame.RecordFrame(1, 9);
            //newGame.RecordFrame(10, 0);
            //newGame.RecordFrame(0, 0);
            //newGame.RecordFrame(10, 10, 0);

            // Test successful (236)
            //newGame.RecordFrame(3, 7);
            //newGame.RecordFrame(9, 1);
            //newGame.RecordFrame(10, 0);
            //newGame.RecordFrame(10, 0);
            //newGame.RecordFrame(8, 2);
            //newGame.RecordFrame(10, 0);
            //newGame.RecordFrame(10, 0);
            //newGame.RecordFrame(9, 1);
            //newGame.RecordFrame(10, 0);
            //newGame.RecordFrame(10, 10, 10);

            // All spares + bonus in 10th (150)
            newGame.RecordFrame(1, 9);
            newGame.RecordFrame(2, 8);
            newGame.RecordFrame(3, 7);
            newGame.RecordFrame(4, 6);
            newGame.RecordFrame(5, 5);
            newGame.RecordFrame(6, 4);
            newGame.RecordFrame(7, 3);
            newGame.RecordFrame(8, 2);
            newGame.RecordFrame(9, 1);
            newGame.RecordFrame(0, 10, 6);



            string finalScore = "\nThe FINAL score is : " + newGame.Score;
            newGame.ColorMessage(ConsoleColor.Red, finalScore);
        }
    }

}

A couple days later I heard feedback from them that basically they were disappointed with the code that I turned in, but they didn't say more than that.

Can someone tell me why? Did I fail to understand what they were really looking for? I thought my code was good because it met all of their stated requirements and does what it's supposed to.

What am I missing?

\$\endgroup\$
  • \$\begingroup\$ With what language do you have the one year experience? \$\endgroup\$ – Denis Oct 14 at 13:47
  • 1
    \$\begingroup\$ What am I missing? The question is not really a good test of C# knowledge. It lacks a description of how bowling scores work. While the ability to research bowling scores might be a useful skill, it's not a C# skill and candidates familiar with bowling are at an advantage over those who are not. The hiring team was too lazy to actually describe the problem. Wasting your time and not providing useful feedback is just an extension of that. There are better employers out there. Good luck. \$\endgroup\$ – ben rudgers Oct 14 at 14:51
  • \$\begingroup\$ @Denis, this problem would have been extremely easy for me to accomplish in Python or JavaScript. I would also be able to complete it in Perl or PHP. I love using data structures for things like this. While I do understand OOP, what I don't understand well is unstated expectations. For example, the expectation that the code make use of more classes, or be highly optimized (or elegant). I was just trying to meet the requirements with working code. They know I don't yet know C#. Optimization could come later. \$\endgroup\$ – Chris Nielsen Oct 14 at 17:41
  • \$\begingroup\$ @benrudgers is right if they give you the Bowling Kata as test there should be some more information on how to solve it and what they expect. \$\endgroup\$ – Markus Deibel Oct 15 at 5:20
  • \$\begingroup\$ I noticed a common theme with a lot of the responses I got on this Code Review. C# developers tend to view their code almost as art! I see the terms elegant, clear, clean, production quality, etc.. One person was bothered that my ternaries weren't lined up in the same order! This is a really different culture than I am used to. As a utilitarian developer, my focus is almost always on code that works. I use a lot of Python, and I do try to adhere to PEP 8 standards, so I guess I am used to what looks good in Python but am just naive about what does or doesn't look good in C#. \$\endgroup\$ – Chris Nielsen Oct 15 at 18:38
9
\$\begingroup\$

Inconsistency

This is a minor thing, but I tripped over it when skimming your code:

throw2 = (throws.Length == 1) ? 0 : throws[1];
throw3 = (throws.Length == 3) ? throws[2] : 0;

Why did you invert one of the ternaries? It'd be easier to read if you kept the same structure, e.g.

throw2 = (throws.Length >= 2) ? throws[1] : 0;
throw3 = (throws.Length >= 3) ? throws[2] : 0;

Variable juggling

You're spending a whole lot of effort on putting things in other locations and then putting them back. Look at what happens to the throw data:

public void RecordFrame(params int[] throws)
{
    throw1 = throws[0];
    // In case there is no second number (0) provided when someone throws a strike
    throw2 = (throws.Length == 1) ? 0 : throws[1];
    throw3 = (throws.Length == 3) ? throws[2] : 0;
    frameScore = throw1 + throw2;
    // Store the frame scores in a list as they come in, to be updated with strike and spare bonuses
    tenFrameScores.Add(frameScore);

    // Dynamically populate the JaggedArray tenFrames
    if (throws.Length == 3)
    {
        tenFrames[frame] = new[] { throw1, throw2, throw3 };
    } 
    else
    {
        tenFrames[frame] = new[] { throw1, throw2 };
    }

    // ...
}
  1. You get an array of throws
  2. You define a custom variable for each element of the array
  3. You then merge these custom variables into an array to store them.

There's no point to making these separate variables. There is no coding overhead compared to e.g. calling throws[0] (which you already have) as opposed to throw1 (which you need to create.

This can be rewritten to a much simpler:

public void RecordFrame(params int[] throws)
{
    tenFrameScores.Add(throws.Take(2).Sum());
    tenFrames[frame] = throws;

    // ...
}

To be fair, what I have omitted here is the default 0 assignment to unmentioned throws which may be important down the line, but that is something I will tackle in a subsequent feedback point as I suggest reworking this much further. For the currently mentioned code, it will not throw an exception even if only one throw value was entered.


Storing empty throws

I disagree that you should store empty throws. I don't mean gutter balls, but rather never-thrown values, such as the second throw in a strike frame, or the third throw in the 10th frame if there was no spare or strike.

By storing them as a 0 value, you make it harder on yourself to later tell the difference between a gutter ball (an actual 0 throw) or a non-existent throw.

The reason you should avoid this is that it complicates your strike bonus calculation. Very basically, the logic of a strike is that you add the value of the next two throws to the strike's score.
Now, if the next frame is also a strike, and you were to store this second frame as (10,0), your logic would think that those were the "next two" throws.

That is not the case. The second throw does not exist and does not count.

Compare this to if the second frame is (0,0) (two gutter balls). Here, the second throw does exist and it does count towards the "next two throws" after the strike.

By not storing the non-existent throws as zeroes, you could severely cut down on the complexity of your scoring calculation, as you wouldn't need to handle things like "is the second throw also a strike" fringe cases.


No validation

You aren't really validating any of the input. There are some simple validation checks you could be implementing:

  • Making sure at least 1 throw value is registered for each frame
  • Making sure no more than 2/3 values are registered for each frame (based on whether it's the 10th frame or not)
  • Making sure no frame contains more than 10 total score (unless it's the 10th frame, in which case 30 is the maximum)
  • Making sure no single throw value is lower than 0
  • Making sure no single throw value is higher than 10

Validation can be short or elaborate, and we can argue and disagree which validation is necessary or superfluous; but as a job interviewer I would want to see some validation in the code as a matter of good practice.


Lists vs arrays

private readonly List<int> throws = new List<int>(21); // 21 is the maximum amount of throws.

One the the main sticking point of using a List<> over an array is that you do not need to pre-emptively specify the collection size. Lists can grow dynamically. It makes little sense to specify a maximum size here.

To be fair, I find it weird that the language allows for it, but I suspect this is one of those "it could be done even though no one really needs it" instances, the .Net framework has a few of those.


Expected usage inconsistency

I noticed this in your test cases:

// Test perfect game - SUCCESS! (300)
newGame.RecordFrame(10);
newGame.RecordFrame(10, 0);
newGame.RecordFrame(10);

I disagree that (10,0) should be a valid throw. You are not allowed to throw a second time in a frame if the first throw was a strike. It's a counterintuitive approach.

That being said, I also slightly disagree with the interface (which is not your fault but I'd still like to point it out), it makes more sense to use explicit parameters with default values here, e.g.:

void RecordFrame(int throw1, int throw2 = 0, int throw3 = 0);

This enforces that a frame always consists of 1 to 3 throws. Using an int[] allows for 0 or more than 3 values, which is plain wrong.

I know you can't change the requirement you were given, but I think it is still useful to point it out as an improvement nonetheless.


When to calculate the end score

I disagree with the location you've chosen to do the calculations:

public void RecordFrame(params int[] throws)
{
    // Record frame logic

    if (numberFramesSoFar == 10)    // Ready to calculate a score
    {
        // Scoring logic
    }
}

Recording frames and calculating scores are two completely separate responsibilities. As an absolute minimum, these should've been separated into separate methods just to break up the monolithic method you've ended up with. E.g.:

public void RecordFrame(params int[] throws)
{
    // Record frame logic

    if (numberFramesSoFar == 10)    // Ready to calculate a score
    {
        CalculateFinalScore();
    }
}

public void CalculateFinalScore()
{
    // Scoring logic
}

But I also disagree with triggering the score calculation during the recording of the tenth frame. It's much more intuitive to do this when the score is being observed:

public void RecordFrame(params int[] throws)
{
    // Record frame logic
}

public int CalculateFinalScore()
{
    // Scoring logic
}

public int Score => CalculateFinalScore();

Note that you could cache the calculated score so that accessing the property a second time doesn't retrigger a recalculation, but I'm going to currently dismiss that as a "nice to have" based on more pressing issues with the code you've currently posted.

Nonetheless, if you are interested, here's a basic implementation:

public void CalculateFinalScore()
{
    // Scoring logic

    _calculatedScore = // result from calculation
    _scoreWasCalculated = true;
}

private bool _scoreWasCalculated = false;
private int _calculatedScore = 0;

public int Score 
{
    get
    {
        if(!_scoreWasCalculated)
            CalculateFinalScore();

        return _calculatedScore;
    }
}

Lack of OOP

The biggest issue I see in the codebase is that you are not using OOP.

I won't rehash JAD's answer, but he is correct about the public/private issues and some of the scope declarations (class variables vs local variables).

One thing I think hasn't been pointed out yet is that your entire logic relies on arrays and primitives. The only OOP object you've used is the BowlingGame class, which you were forced to use because of the interface in the requirements.

They were willing to give me a shot because I am really good at learning new programming languages and am otherwise a solid developer.

Without trying to be mean, I'm going to disagree here. The low readability of your code is something that is independent of C#. Whatever your existing experience is in, I suspect your code is going to be of equally low readability, which is not what I would associate with a "solid developer".

Please don't take this as an attack - I genuinely don't mean to insult. There's nothing wrong with being a starter. Everyone has to start somewhere, we all had to learn the ropes, I've written worse C# code than yours when I was just beginning. But I would advise you against overselling yourself.
If you overstate your ability and then fail to meet that claim, you're going to be less likely to be hired (if I were your interviewer) compared to if you are humble about your ability but put a genuine effort forward (even if you end up with mistakes).

As this was a job interview, as an interviewer I would conclude that you do not grasp the basics of OOP and are not a good fit for the position of a C# developer. A one year experience requirement is not very much, but in one year a developer should at least be able to apply OOP principles.

You can't be expected to apply OOP perfectly (SOLID etc) even if you had the required one year of experience, but you should at least be using it and in this exercise you didn't really try to implement OOP.

Without intending to sound mean, I can't explain the entirety of OOP and how it could be used for your example, but I will try to get you started on what you can look at (I suggest following some OOP tutorials as StackExchange cannot provide the same level of guidance)

As a basic example, a Frame class would come to mind, where you store the throw information. This allows you to not only work with neatly separated data objects, it also allows for reusable logic such as IsStrike or IsSpare checks.

A rough draft (this is open to many improvements, I'm trying to give you the simplest example to get started):

public class Frame
{
    public List<int> Throws { get; private set; }

    public Frame(List<int> throws)
    {
        if(throws == null || throws.Count() < 1 || throws.Count() > 3)
            throw new ArgumentException("Frames must consist of 1 to 3 throws", "throws");

        if(throws.Any(throw => throw > 10))
            throw new ArgumentException("A single throw cannot contain more 10 pins");

        this.Throws = throws;
    } 

    public bool IsSpare()
    {
        return this.Throws.Take(2).Sum() == 10;
    }

    public bool IsStrike()
    {
        return this.Throws.First() == 10;
    }

}

This means you can change all your "value equals 10" checks with a much more readable call to the IsStrike() and IsSpare() methods. Not only is it more readable, it's also more refactor-friendly (though admittedly I don't expect the rules of bowling to be changed anytime soon, the point still stands on principle).

I suggest you follow up with some C# OOP tutorials to learn the finer points of OOP and how to implement it.


Can someone tell me why? Did I fail to understand what they were really looking for? I thought my code was good because it met all of their stated requirements and does what it's supposed to.

The code doing what it's supposed to do is not the only measure of a good developer. I can write massively different code files that are treated exactly the same by a compiler, yet are dramatically different in terms of quality.

As a silly but blatant example:

private int poizpdodz(int pdnzfe, int poknsdds)
{
     return pdnzfe + poknsdds;
}

private int Add(int a, int b)
{
     return a + b;
}

The compiler does not care about method/variable names and both methods will be compiled (and executed) the exact same way, but we can all agree that the second method is much better written.

Furthermore, it's not just about how the code works now, but also how easy it is to maintain the code in the future. A lot of code in the average codebase I work in is not related to the business goal of the specific application, but rather framework/architecture than ensures that any changes that need to be made in the future can be minimized in terms of effort and impact.

In short, there are many more metrics to the quality of code than just "it works".

\$\endgroup\$
  • \$\begingroup\$ With regards to specifying the length of the list of throws on creation; I'll admit that it's a micro-optimisation, but just because List<T> supports growing lists, doesn't mean we should make use of it. There is an overhead involved in growing the internal array, so if we can pre-empt that by knowing the theoretical max size (and min size), why not? \$\endgroup\$ – JAD Oct 14 at 18:11
  • \$\begingroup\$ I think the tricky part of creating a Frame class is that it gains you little for calculating the score of the frame, due to the special status of strikes and spares. Minor nitpick: maybe expose Frame.Throws as a IReadOnlyList. \$\endgroup\$ – JAD Oct 14 at 18:14
  • \$\begingroup\$ That's it for my nitpicks, agreed with the rest. +1 \$\endgroup\$ – JAD Oct 14 at 18:15
  • \$\begingroup\$ Another thing to possibly add is the use of code comments to describe what the code is doing because it is not expressive enough. This ties back into your comment about not having a full grasp of OOP. \$\endgroup\$ – Magrangs Oct 15 at 10:12
5
\$\begingroup\$

Private versus public

    public int frame = 0;           // The current frame
    public int frameScore = 0;      // The total of first two throws in a normal frame
    public int[] throws;            // Input
    public int throw1;              // First throw in a frame
    public int throw2;              // Second throw in a frame
    public int throw3;              // Third throw in the 10th frame

    public List<int> tenFrameScores = new List<int>();
    public int[][] tenFrames = new int[10][]; // a JaggedArray (to allow for three numbers in the 10th frame)

All these fields should be private. Since your class implements the interface ISimpleBowlingGame, the idea is that most, if not all, interaction with this class should be based on that interface. By making these fields public, you're allowing the user to mess with the internal state of the class, which might mess up all logic you have.

The same goes for Score.Set, this should be private; the score is determined by the frames given through RecordFrame(), adjusting the score manually makes no sense.

When to use public/private/internal is a pretty basic part of understanding languages like C#, so be sure to review this some more.


Class fields versus method variables

    public int throw1;              // First throw in a frame
    public int throw2;              // Second throw in a frame
    public int throw3;              // Third throw in the 10th frame

These are only ever used within RecordFrame, so make them local variables within that method.


Console.WriteLine

The spec specifically said that you didn't have to provide a user interface. So all of the printing to console was a wasted effort. Especially with all the different colours and such.

Not even was it unnecessary, but it's not the most flexible implementation either. What if instead of writing the output to console we wanted to write it to a file? Remember that the primary concern of the class is to calculate a bowling score. You don't know how it is going to be used yet, or where. So it's best to not make assumptions to that.

If you want to provide a way to turn the state of the class into a string, so that the user could choose to print it to commandline, provide an implementation of ToString(), which you can override:

public override string ToString() {
  // implementation;
}

Scoring

Your scoring code is a bit of a mess. Let's quickly go over the rules:

  1. A throw is worth at least the number of pins thrown.
  2. If a strike is thrown (10 pins in the first throw of a frame), the next two throws are added to the score of the strike.
  3. If a spare is thrown (10 pins in the frame, not a strike), the next throw is added to the score of the spare.
  4. If the tenth frame ends in a strike or spare, 2 or 1 throws are added respectively to add the bonus score, these don't count on their own.

Given that the complexity of the challenge is in the scoring mechanism, this should be leading for how you store the throws. I don't think that separating the throws in frames in tenFrames is the best approach.

Why? Because to score spares and strikes, you need to be able to look ahead to the next throws to determine the score. This is easiest if the next throws are adjacent in an array, instead of in a jagged array. Remember that if you throw 2 strikes in a row, you need to look two frames ahead to determine the score of the first strike!

So let's assume all throws are in an int[], the first frame could then be scored as such:

if(throws[0] == 10) {
  // strike
  score += 10 + throws[1] + throws[2];
} else if (throws[0] + throws[1] == 10) {
  // spare
  score += 10 + throws[2];
} else {
  // normal throw
  score += throws[0] + throws[1];
}

That's pretty neat isn't it? Let's loop over all frames:

public int ScoreFrames(int[] throws)
{
  var frame = 1;
  var score = 0;
  var frameStart = 0;
  while (frame <= 10)
  {
    if (throws[frameStart] == 10)
    {
      // strike
      score += 10 + throws[frameStart + 1] + throws[frameStart + 2];
      frameStart += 1;
    }
    else if (throws[frameStart] + throws[frameStart + 1] == 10)
    {
      // spare
      score += 10 + throws[frameStart + 2];
      frameStart += 2;
    }
    else
    {
      // normal throw
      score += throws[frameStart] + throws[frameStart + 1];
      frameStart += 2;
    }
    frame++;
  }
  return score;
}

You can keep track of where the next frame starts in frameStart: a strike is only one throw in a frame, the rest is two.

The rest is simple:

  public class SimpleBowlingGame : ISimpleBowlingGame {
    private readonly List<int> throws = new List<int>(21); // 21 is the maximum amount of throws.
    private int numberOfFrames = 0;

    public int Score { get; private set; }

    public void RecordFrame(params int[] throws) {
      // Optional input validation.

      this.throws.AddRange(throws);
      numberOfFrames++;

      if(numberOfFrames == 10) {
        SetScore();
      }
    }

    private void SetScore() {
      var frame = 1;
      var score = 0;
      var frameStart = 0;
      while (frame <= 10) {
        if (throws[frameStart] == 10) {
          // strike
          score += 10 + throws[frameStart + 1] + throws[frameStart + 2];
          frameStart += 1;
        }
        else if (throws[frameStart] + throws[frameStart + 1] == 10) {
          // spare
          score += 10 + throws[frameStart + 2];
          frameStart += 2;
        }
        else {
          // normal throw
          score += throws[frameStart] + throws[frameStart + 1];
          frameStart += 2;
        }
        frame++;
      }
      Score = score;
    }
  }

Bonus points: input validation

Spec says it's not necessary, but it never hurts to verify that the throws you get as input are actually legal. In the SetScore method, we rely heavily on the inputs as we expect them to be, so it doesn't hurt to validate:

  1. All numbers in throws are between 0 and 10
  2. If the first number is lower than 10, the second can at most be 10 - throws[0]
  3. throws.Length is 1 or 2, unless it's the tenth frame, then it can only be 3 if it's a spare or strike, else it's 2.
  4. After the tenth frame, no more input is taken.

Bonus points: Disallow Score from being called before the tenth frame is provided.

private int score;
public int Score {
  get {
    if (numberOfFrames != 10) {
      throw new InvalidOperationException("Can't provide score before 10 frames are recorded");
    }
    return score;
  }
  private set {
    score = value;
  }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ I find it counterintuitive to throw an exception on the score when the game is in progress, as opposed to showing the current score. It's an unjustified exception usage, imho. Every bowling score tracker I've seen shows you the current score (and yes, you can't account for the potential bonus points on recent strikes and spares and that's okay). \$\endgroup\$ – Flater Oct 14 at 11:14
  • \$\begingroup\$ @Flater that's a design decision that can be justified. The spec says Called at the end of the game to get the final score. and the property isn't filled until the last frame is entered, so in this implementation, showing the score before the tenth frame is not supported. This is, IMO, a good reason to throw, instead of returning default(int) as if everything was ok. \$\endgroup\$ – JAD Oct 14 at 12:48
  • 1
    \$\begingroup\$ @JAD, they said in the instructions: You do not need to handle invalid input in your class. but they never said what was valid! I guess this means that some level of logical assumption IS necessary. Thank you for all the input. I am learning from this. \$\endgroup\$ – Chris Nielsen Oct 14 at 19:33
  • 1
    \$\begingroup\$ @JAD It looks like you are using one long array called throws to store all of the throws and are abandoning the frame distinction in RecordFrame. Your idea is to recreate frames using logic in the SetScore function? So if someone wanted to create a "print game" function later, showing the throws in each frame, they would have to recreate the game using logic rather than just printing the game based on frames and throws that were originally input? I am normally in favor of using data structures that mimic the input, rather than recreating the data using logic. \$\endgroup\$ – Chris Nielsen Oct 17 at 16:54
  • 1
    \$\begingroup\$ @JAD Even in OOP, we want to maintain data integrity right? Coming from Python, an array of tuples would be perfect for this. \$\endgroup\$ – Chris Nielsen Oct 17 at 16:55
1
\$\begingroup\$

To the many useful points already made I have only a small contribution to add:

    public class Test
    {
        public static void Main()
        {
            BowlingGame newGame = new BowlingGame();

            // Test first game - SUCCESS! (190)
            //newGame.RecordFrame(4, 6);
            //newGame.RecordFrame(3, 6);
            //newGame.RecordFrame(2, 1);
            //newGame.RecordFrame(10); // 30
            //newGame.RecordFrame(10); // 30
            //newGame.RecordFrame(10); // 21
            //newGame.RecordFrame(10); // 20
            //newGame.RecordFrame(1, 9);
            //newGame.RecordFrame(10,0);
            //newGame.RecordFrame(10, 4, 6);

That's not an elegant way of doing tests. The "best practice" would be to use a testing framework like NUnit or Microsoft's testing SDK.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the info. I've never heard of those as C# and the Microsoft stack are brand new to me. \$\endgroup\$ – Chris Nielsen Oct 15 at 5:01
  • \$\begingroup\$ @ChrisNielsen, for what it's worth, lots of languages have something similar. If you get a similar coding task for a Java interview, for example, I'm guessing that JUnit is still the de facto standard. \$\endgroup\$ – Peter Taylor Oct 15 at 9:54

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.