I made a little mastermind game in C# to exercise - do I take the right approach?

I'm very new in C#, and the best way to learn, is to do exercises. As I tried to find a challenging exercise, I created this little game called MasterMind. The rules are very simple, and are explained on numerous places (i.e. here). You can find below my version in C# of this game, but written only for the console application, so there's no fancy nice layout using Forms or WPF. I wanted to concentrate on the code. If anyone's interested, can you tell me if I made the right approach? For example, I created a new class called Ball, with just a color field and a method to choose a random color. I probably over-complicated this, but I wanted to work with Classes. Do you have any remarks on naming conventions, other things, ...

If anyone has suggestions to code better, I'm all ears :-)

You can find the code on my Github (https://github.com/TjerkBeke/MasterMind.git), or here beneath...

Program.cs

internal class Program
{
public static void Main(string[] args)
{
// The number of balls you can pick. The higher the number, the more difficult the game.
int NumberOfBalls = 4;
// The number of chances you get. The lower, the more difficult the game.
int NumberOfChances = 12;

string Outcome = "";

// Take a random order of balls to guess
var BallsToGuess = new List<Ball>();
for (int i = 0; i < NumberOfBalls; i++)
{
}

// Show the computer's choice. Uncomment this section to cheat ;-)
//Console.WriteLine("The computer chose the following balls");
//Console.WriteLine(ConvertBallColorToString(BallsToGuess));

// Show the intro and game rules
ShowIntro();

// Loop until you have no more chances left
for (int i = 0; i < NumberOfChances; i++)
{
Console.WriteLine($"Chances left: {NumberOfChances - i}"); // Choose your balls Console.WriteLine("Provide your choice:"); List<Ball> BallsPicked = GetYourBalls(BallsToGuess.Count); // Calculate the exact and non-exact matches. An exact match gicves you a black pin, a non-exact match gives you a white pin int[] ReturnedPins = CalculatePins(BallsToGuess, BallsPicked); // Show the results to the screen, and show the previous results as well. Outcome = GetOutcome(ReturnedPins, BallsPicked, Outcome); Console.Clear(); Console.WriteLine(Outcome); // Check if you won if (ReturnedPins[0] == 4) { Console.WriteLine("Congratulations!! You have guessed the correct answer!"); Console.WriteLine($"You had { NumberOfChances - i } chances left");
return;
}
}

// If you come to here, it means you didn't win :-(
Console.WriteLine("Sorry, but you didn't find the correct solution within the needed number of chances.");
Console.WriteLine("The computer chose the following balls");
Console.WriteLine(ConvertBallColorToString(BallsToGuess));

}

#region Methods
/// <summary>
/// Shows the different pins, and the chosen ball collection.
/// </summary>
/// <param name="returnedPins"></param>
/// <param name="ballsPicked"></param>
private static string GetOutcome(int[] returnedPins, List<Ball> ballsPicked, string formerAttempt)
{
string ballColors = ConvertBallColorToString(ballsPicked);
string pinColors = ConvertPinColorToString(returnedPins);
return formerAttempt + (pinColors + ballColors) + "\n";

}
/// <summary>
/// Takes an array of 2 integers, and transforms them into a string with the number of characters defined in the integers.
/// # is the number of black pins.
/// | is the number of white pins.
/// </summary>
/// <param name="ReturnedPins"></param>
/// <returns></returns>
private static string ConvertPinColorToString(int[] ReturnedPins)
{
string PinList = "";

string Black = new string('#', ReturnedPins[0]);
string White = new string('|', ReturnedPins[1]);

return (\$"'{PinList}'");
}

/// <summary>
/// Shows the goal of the game and the rules.
/// </summary>
private static void ShowIntro()
{
Console.WriteLine("Welcome to this game of MasterMind.");
Console.WriteLine("The goal of the game is to guess what colors and the correct order of colors I have hidden.");
Console.WriteLine("You can choose between the colors: BLUE - RED - GREEN - WHITE - PINK - YELLOW");
Console.WriteLine("You provide input by taking the first letters of the colors; so for example for the color yellow, you need to press the Y-key.");
Console.WriteLine("After every guess, I will tell you:");
Console.WriteLine("The number of correct colors in the correct place: a black pin");
Console.WriteLine("The number of correct colors not in the correct place: a white pin");
Console.WriteLine("Press a key to start");
Console.Clear();
}

/// <summary>
/// Asks for the balls you want to pick, using the readkey function, and creates a List of balls.
/// </summary>
/// <param name="NumberOfBalls"></param>
/// <returns></returns>
private static List<Ball> GetYourBalls(int NumberOfBalls)
{
var yourBalls = new List<Ball>();

for (int i = 0; i < NumberOfBalls; i++)
{

switch (yourBallColor)
{
case ConsoleKey.W:
break;
case ConsoleKey.R:
break;
case ConsoleKey.G:
break;
case ConsoleKey.B:
break;
case ConsoleKey.P:
break;
case ConsoleKey.Y:
break;
default:
break;
}

}

return yourBalls;
}

/// <summary>
/// Calculates the resulting pins of your guess. An exact match is a black pin, a non-exact match is a white pin.
/// </summary>
/// <param name="ballsToGuess"></param>
/// <param name="ballsPicked"></param>
/// <returns></returns>
private static int[] CalculatePins(List<Ball> ballsToGuess, List<Ball> ballsPicked)
{
int blackPin = 0;
int whitePin = 0;

// First of all, create a duplicate of both ball objects,
// as we don't want to change the balls of the original list
var duplicateBallsToGuess = new List<Ball>();
for (int ballIndex = 0; ballIndex < ballsToGuess.Count; ballIndex++)
{
}

var duplicateBallsPicked = new List<Ball>();
for (int ballIndex = 0; ballIndex < ballsPicked.Count; ballIndex++)
{
}

// First check all exact matches
for (int i = 0; i < duplicateBallsPicked.Count; i++)
{
// check if there is an exact match
if (duplicateBallsToGuess[i].Equals(duplicateBallsPicked[i]))
{
// Black pin, as it is found, and on the exact place
blackPin++;
duplicateBallsToGuess[i].Color = BallColor.None;
duplicateBallsPicked[i].Color = BallColor.None;
continue;
}
}

// Now check all non-exact matches
for (int i = 0; i < duplicateBallsPicked.Count; i++)
{
if (duplicateBallsPicked[i].Color == BallColor.None)
{
// Ball is already mapped to an exact match
continue;
}
// Find the index of color to look for; index of -1 means it is not found
int index = duplicateBallsToGuess.FindIndex(ball => ball.Color == duplicateBallsPicked[i].Color);
if (index == -1)
{
// Nothing found, so moving to the next
continue;
}
// White pin, as it is found, but not on the exact place
duplicateBallsToGuess[index].Color = BallColor.None;
whitePin++;
}
int[] ReturnedPins = { blackPin, whitePin };
return ReturnedPins;
}

/// <summary>
/// Converts a list of object balls into a string, where the colors are the first letters of their color name
/// </summary>
/// <param name="balls"></param>
/// <returns></returns>
public static string ConvertBallColorToString(List<Ball> balls)
{
string ballcolors = "";
for (int i = 0; i < balls.Count; i++)
{
switch (balls[i].Color)
{
case BallColor.White:
ballcolors += "W ";
break;
case BallColor.Red:
ballcolors += "R ";
break;
case BallColor.Blue:
ballcolors += "B ";
break;
case BallColor.Green:
ballcolors += "G ";
break;
case BallColor.Yellow:
ballcolors += "Y ";
break;
case BallColor.Pink:
ballcolors += "P ";
break;
default:
ballcolors += "X ";
break;
}
}

return ballcolors.Trim();
}
#endregion

}


Ball.cs

internal class Ball
{
#region Properties
/// <summary>
/// The color you want the ball to have
/// </summary>
public BallColor Color { get; set; }

#endregion

#region Constructors
/// <summary>
/// Empty Constructor - creates a ball with a random color
/// </summary>
public Ball()
{
Color = GetRandomColor();
}
/// <summary>
/// Constructor - creates a ball with a predefined color
/// </summary>
/// <param name="Color"></param>
public Ball(BallColor color)
{
Color = color;
}

#endregion

#region Methods
/// <summary>
/// Picks a random color
/// </summary>
public BallColor GetRandomColor()
{
// Convert the color enumeration to a list, as this is easier to work with
List<BallColor> ballColors = Enum.GetValues(typeof(BallColor)).Cast<BallColor>().ToList();
// Remove the first value from the color enumaration, as it is not a color
ballColors.Remove(BallColor.None);
// Pick a random value
Random random = new();
return ballColors[random.Next(ballColors.Count)];
}
#endregion

/// <summary>
/// Override the Equal method, as we want to comapre ball colors
/// </summary>
/// <param name="obj"></param>
/// <returns></returns>
public override bool Equals(object obj)
{
// If the passed object is null
if (obj == null)
{
return false;
}
if (!(obj is Ball))
{
return false;
}
return (this.Color == ((Ball)obj).Color);
}
public override int GetHashCode()
{
return Color.GetHashCode();
}

}


enum.cs

public enum BallColor
{
None,
White,
Red,
Blue,
Green,
Pink,
Yellow
}


static

Members that do not make use of instance members should be static.

For example, Ball.GetRandomColor() does not make any use of instance state or instance methods (in other words, it never uses this), so it should be static:

public static BallColor GetRandomColor()


This also applies to the Program class as a whole: it is never instantiated, and it only has static members, so it can be static as well:

internal static class Program


private

Members that do not need to be accessed from the outside should be private.

For example, Ball.GetRandomColor and Program.ConvertBallColorToString are never accessed from the outside, so they should be private:

private static string ConvertBallColorToString(List<Ball> balls)

private static BallColor GetRandomColor()


Properties

To me, GetRandomColor looks more like a property than a method. So, I would make it a property:

public static BallColor RandomColor
{
get
{
// Convert the color enumeration to a list, as this is easier to work with
List<BallColor> ballColors = Enum.GetValues(typeof(BallColor)).Cast<BallColor>().ToList();
// Remove the first value from the color enumaration, as it is not a color
ballColors.Remove(BallColor.None);
// Pick a random value
Random random = new();
return ballColors[random.Next(ballColors.Count)];
}
}


The callsite also needs to be updated accordingly:

Color = RandomColor


Note: this is somewhat controversial since properties should not have side-effects, but this property uses a PRNG. I can see both sides of the argument.

PRNG management

You are creating a new PRNG on every call to GetRandomColor. That is not only wasteful, but can also lead to bugs: the PRNG is seeded by default with a time-based seed. So, if you create PRNGs fast enough after each other, they will have the same seed and thus produce the same sequence of "random" numbers!

You should only create the PRNG once and reuse it. I would simply store it in a private static field:

private static readonly Random random = new();

// Pick a random value
return ballColors[random.Next(ballColors.Count)];


Expression-bodied members

For members whose body consists only of a single expression, there is an alternative syntax available which removes some "cruft". Ball.GetHashCode and the two Ball constructors fall into this category:

public Ball() => Color = RandomColor;

public Ball(BallColor color) => Color = color;

public override int GetHashCode() => Color.GetHashCode();


this constructor initializer

You can use a this constructor initializer to delegate from one constructor to another. In this case, we can delegate from the no-args constructor to the one that takes an argument:

public Ball() : this(RandomColor) { }


For this simple example, there is not much of a difference whether we call the constructor ourselves in the constructor body or if we let the compiler do it. But for more complex hierarchies with more complex initialization, this can make it much clearer to see what is going on.

Generic Enum.GetValues<T>

You can remove all the typeof and Casting from the conversion of the enum values to a list by using the generic version of Enum.GetValues<T>.

Before:

List<BallColor> ballColors = Enum.GetValues(typeof(BallColor)).Cast<BallColor>().ToList();


After:

List<BallColor> ballColors = Enum.GetValues<BallColor>().ToList();


Simpler way to pick a random color

If you move the None value to the end of enum BallColor, then, instead of filtering it out of a list, you can simply ignore the last element.

Before:

// Convert the color enumeration to a list, as this is easier to work with
List<BallColor> ballColors = Enum.GetValues<BallColor>().ToList();
// Remove the first value from the color enumaration, as it is not a color
ballColors.Remove(BallColor.None);
// Pick a random value
return ballColors[Random.Next(ballColors.Count)];


After:

var ballColors = Enum.GetValues<BallColor>();
// Pick a random value
return ballColors[random.Next(ballColors.Length - 1)];


record classes

For simple data objects like Balls, C# has record classes and record structs. Converting Ball to a record class is relatively simple:

1. Add the record keyword to the class declaration:
internal record class Ball

2. Delete GetHashCode and Equals. They are auto-generated for us, based on the properties of the record. (We also get auto-generated ==, != and an implementation of IEquatable, ToString(), and many others.)

Positionalrecord class

We can even go one step further and make our record a positional record. Positional records declare positional members as part of their record declaration and get additional auto-implemented members for those (for example, constructors).

We can make Ball a positional record by adding the Color member as a positional member to the declaration:

internal record class Ball(BallColor Color)


Now we can simply delete the Ball(BallColor) constructor: it will be autogenerated for us.

We can not delete the property declaration, since positional record classes by default generate init-only properties, but we need our property to be settable. According to the rules of records, we also need to initialize our property from the positional member:

public BallColor Color { get; set; } = Color;


Naming conventions for parameters and local variables

Constructor and method parameters as well as local variables should use camelCase naming, not PascalCase.

This applies to NumberOfBalls, NumberOfChances, Outcome, BallsToGuess, BallsPicked, ReturnedPins, Black, White, and PinList.

Prefer string.Empty over "" for empty strings

You should prefer the string.Empty property over an empty string literal for empty strings, for example here:

string outcome = string.Empty;


Unnecessary parentheses

The parentheses here are unnecessary:

return formerAttempt + (pinColors + ballColors) + "\n";


You can just remove them:

return formerAttempt + pinColors + ballColors + "\n";


Implicit typing using var

This is a controversial topic. I, personally, am a big fan of implicit typing / type inference using var. Whenever the type of a variable is obvious in context, I don't see why it should be spelled out.

So, I would write

var numberOfBalls = 4;

var numberOfChances = 12;

var outcome = string.Empty;

for (var i = 0; i < numberOfChances; i++)

var ballsPicked = GetYourBalls(ballsToGuess.Count);

var pinList = string.Empty;

for (var i = 0; i < numberOfBalls; i++)

var blackPin = 0;
var whitePin = 0;

for (var ballIndex = 0; ballIndex < ballsToGuess.Count; ballIndex++)

for (var ballIndex = 0; ballIndex < ballsPicked.Count; ballIndex++)

for (var i = 0; i < duplicateBallsPicked.Count; i++)

for (var i = 0; i < duplicateBallsPicked.Count; i++)

var index = duplicateBallsToGuess.FindIndex(ball => ball.Color == duplicateBallsPicked[i].Color);

var ballcolors = string.Empty;
for (var i = 0; i < balls.Count; i++)


But others may (and will) disagree.

Target-typed new

I am also a big fan of target-typed new in cases where the type is obvious from context.

For example, here:

string black = new('#', returnedPins[0]);
string white = new('|', returnedPins[1]);


Unnecessary local variable

I feel like the local variable here does not add much to readability:

int[] returnedPins = { blackPin, whitePin };
return returnedPins;


I would inline it:

return new int[] { blackPin, whitePin };


Namespaces

All types should be in namespaces, and there should be one top-level namespace for your project. (I notice you are actually doing this in your GitHub repository but not in the code you posted here.)

As of C# 10, file-level namespaces allow us to get rid of the annoying extra level of nesting.

namespace MasterMind;



to the top of every file.

Unnecessary early declaration / initialization

In Program.ConvertPinColorToString, pinList is declared and initialized at the top of the method, but never used until later, where it is immediately assigned to, thus overwriting the value it was initialized with.

This makes the initialization unnecessary, and it can be removed. But even better, we can move the whole declaration of the variable down to where it is first used:

var pinList = (black + white).PadRight(4);


StringBuilder

Repeatedly concatenating strings is inefficient, since a new string is allocated for each concatenation. While I am generally a fan of purity and referential transparency, unfortunately, the implementation of the string datatype is not optimized for this kind of usage. (A Rope-based implementation would be much more efficient in this regard, for example.)

That's why we have System.Text.StringBuilder:

private static string ConvertBallColorToString(List<Ball> balls)
{
System.Text.StringBuilder ballcolors = new(8, 8);
for (var i = 0; i < balls.Count; i++)
{
switch (balls[i].Color)
{
case BallColor.White:
ballcolors.Append("W ");
break;
case BallColor.Red:
ballcolors.Append("R ");
break;
case BallColor.Blue:
ballcolors.Append("B ");
break;
case BallColor.Green:
ballcolors.Append("G ");
break;
case BallColor.Yellow:
ballcolors.Append("Y ");
break;
case BallColor.Pink:
ballcolors.Append("P ");
break;
default:
ballcolors.Append("X ");
break;
}
}

return ballcolors.ToString().Trim();
}


Pattern Matching

C# supports Pattern Matching, which can make Program.ConvertBallColorToString and Program.GetYourBalls easier to read:

private static List<Ball> GetYourBalls(int numberOfBalls)
{
var yourBalls = new List<Ball>();

for (var i = 0; i < numberOfBalls; i++)
{

Ball ball = new(yourBallColor switch
{
ConsoleKey.W => BallColor.White,
ConsoleKey.R => BallColor.Red,
ConsoleKey.G => BallColor.Green,
ConsoleKey.B => BallColor.Blue,
ConsoleKey.P => BallColor.Pink,
ConsoleKey.Y => BallColor.Yellow,
_            => BallColor.None,
});
}

return yourBalls;
}

private static string ConvertBallColorToString(List<Ball> balls)
{
System.Text.StringBuilder ballcolors = new(8, 8);
for (var i = 0; i < balls.Count; i++)
{
var colorCode = balls[i].Color switch
{
BallColor.White  => "W ",
BallColor.Red    => "R ",
BallColor.Blue   => "B ",
BallColor.Green  => "G ",
BallColor.Yellow => "Y ",
BallColor.Pink   => "P ",
_                => "X ",
};
ballcolors.Append(colorCode);
}

return ballcolors.ToString().Trim();
}


Use the enum member names for the color strings

The string that is used for the color of the pins is actually mostly the first letter of the corresponding enum member. The only exception is None whose string representation is X.

An easy way to make this more regular and remove the special case would be to rename BallColor.None to BallColor.XNone or maybe change the representation to _ and rename to _None.

Once we do that, converting from the enum value to the string representation becomes much simpler:

private static string ConvertBallColorToString(List<Ball> balls)
{
System.Text.StringBuilder ballcolors = new(8, 8);
for (var i = 0; i < balls.Count; i++)
{
var colorCode = balls[i].Color.ToString().Substring(0, 1);
ballcolors.Append(colorCode + "\n");
}

return ballcolors.ToString().Trim();
}


Some OOP

Program does a lot of work to convert Balls to strings. This logic should be part of Ball itself. We can simply provide an override of ToString in Ball:

public override string ToString() => Color.ToString().Substring(0, 1);


and then Program.ConvertBallColorToString becomes just

private static string ConvertBallColorToString(List<Ball> balls) => string.Join(" ", balls);


And now, we are not using the fact that balls is a List anymore, we are only using it as an IEnumerable, so let's change the signature:

private static string ConvertBallColorToString(IEnumerable<Ball> balls) => string.Join(" ", balls);


Tuples

The return value of CalculatePins is somewhat awkward. It is an array of integers, but unlike a normal array where every element has the same "meaning", here the elements mean different things. And the different meanings are only expressed through their index, but there is no "name" attached to the meaning.

This would be much better expressed through a Tuple with named elements.

Let's change the signature of Calculate Pins to return a Tuple:

private static (int blackPins, int whitePins) CalculatePins(List<Ball> ballsToGuess, List<Ball> ballsPicked)


Of course, we also need to change the return value accordingly:

return (blackPin, whitePin);


We need to change the callsite as well. In Main, let's change

int[] returnedPins = CalculatePins(ballsToGuess, ballsPicked);


to

var (blackPins, whitePins) = CalculatePins(ballsToGuess, ballsPicked);


and we also need to change all uses of returnedPins[0] to blackPins and returnedPins[1] to whitePins

For dealing with ConvertPinColorToString and GetOutcome, we have three options:

1. Leave ConvertPinColorToString and GetOutcome alone and call them with an array, e.g. var pinColors = ConvertPinColorToString(new int[] { blackPins, whitePins }).
2. Change the signature of ConvertPinColorToString and GetOutcome to take a Tuple.
3. Change the signature of ConvertPinColorToString and GetOutcome to take the number of black pins and white pins separately.

I decided to go for option #3.

#regions

I am not a big fan of #regions. In general, if your code is so complex that you need to break it down into #regions to understand it, then you should rather break it down into methods and objects.

More precisely, well-written, well-factored, readable code should tell you how the code does things. Well-chosen, intention-revealing, semantic names should tell you what the code does.

The only reason to have comments is to explain why the code does a certain thing in a certain, non-obvious way.

For example, here:

// Show the intro and game rules
ShowIntro();


Do you really need a comment to explain that a method named ShowIntro shows the intro? Especially since the documentation of ShowIntro also says that it shows the intro?

Or here:

// If the passed object is null
if (obj == null)


If someone cannot figure out that if (obj == null) means "if the object is null", then why are they reading C# source code in the first place?

Mixing I/O and computation

There are a couple of places where you are mixing I/O (specifically printing) and computations. Specifically, your Main method.

You should try to keep those separate. Most of your methods should only perform computations. Only some methods at the boundary of the system should print or ask for input.

OOP

Your code is not very object-oriented. Almost everything is static. For example, we are talking about a game of MasterMind here, but there is no Game object to be found!

I would expect, for example, to find at least a Game and Player object, likely a separate object representing GameState, and also some objects for handling input, output, display, and rendering.

And note I wrote objects, not classes. OOP is about objects, it is not about classes. Classes are templates for objects and they are containers for methods, but they are not what OOP is about.

Testing

It is very hard to test your code. It is practically impossible to do any sort of meaningful testing without having to manually play through an entire game. This gets very tedious very quickly, and discourages from testing early and often.

Ideally, tests should be automatic and fast, which allows them to run every couple of seconds after every change in the code.

Overall design

These last points are all intertwined with each other. Testing the logic in the code is hard because the logic is intermixed with the I/O, and so I can't simply call a method and pass in some fake data to test, e.g. the logic for losing the game, I have to actually play through the entire game and lose. I can't automatically play a game by passing in a "fake" player, because there is no such thing as a "player" in the code.

It is also not easy to introduce these changes because most logic is contained in a single Main method.

I would start with breaking up the methods. You already have comments in some places that say something like "first do this", "now do that", "do A", "do B", etc. These comments are telling you that you have separate steps there … so make them separate methods.

As a next step, I would strictly separate I/O and computation. Make sure that all methods take arguments and return values, so that you can easily test them by simply calling them and checking the return value instead of having to play the game.

Then, I would introduce more objects: a game, two players, and whatever else you need. Note that objects represent domain concepts, not real-world physical things. For example, a certain kind of relationship between two objects can itself be an object.

Once you have separated the game logic from the I/O, it should then also be possible to explore building a GUI for the game in WPF or MAUI, or turning the game into a web service that can be played remotely, or turning it into a web app, only by implementing the appropriate interfaces, but without touching the code of your game logic at all.

• Wooow, thanks a lot for this great, great feedback. I'm really impressed you took all this time to go over the code. There are a lot of topics I need to rethink about. This info is very much appreciated! I thought that this 'simple' game would be not too hard to code. But thanks to your comments I do see know more the big picture of coding. Concerning the comments in the code, I just took the best practice that AngelSix is doing in his code: youtube.com/watch?v=uVdGIuVuSmo This off course is subjective :-). Dec 23 '21 at 15:20
• I agree with 98% of your very good review except I do not think GetRandomColor should be a property. Despite Microsoft's own bad practices, properties should be fairly static in nature. Consider cars. Number of doors and Color are decent properties. Number of doors won't change but while Color may change it happens very rarely. Yet something that changes per invocation really should be a method, not a property. Example: something like DateTime.UtcNow really should be a method called GetUtcNow proving even MS does it wrong sometimes. Dec 23 '21 at 16:14
• An issue with comments is that they often tend to rot. Some dev will change the code without changing the attached comment. If I saw the comment mentioned in this answer I'd wonder if there used to be a separate method or if ShowIntro used to do both. To remove that question from my mind, either the method name would need to be changed or the comment. Either way, they'd end up the exact same and redundant. Dec 25 '21 at 3:00