A simple mastermind clone

I made a simple Mastermind clone and I'd just like some tips on what I could do better/different solutions for what I have already coded. If you're wondering what mastermind is, there are, for the original, 6 different colors and 4 different holes. I decided to make this since I had made mastermind in other things so I thought it would be a good starter project in C#.

static void Main(string args)
{
Random GenRandom = new Random();
int t = 0, r, c1 = GenRandom.Next(1, 6), c2 = GenRandom.Next(1, 6), c3 = GenRandom.Next(1, 6), c4 = GenRandom.Next(1, 6);
bool w = false;
string Input, Code = Convert.ToString(c1); Code += c2; Code += c3; Code += c4;
while (t != 8) Input.Contains("7")
End:;
Console.Clear();
if (w == true) Console.WriteLine("You won! The code you guessed was 0.", Code); else Console.WriteLine("You lost! The code you couldnt guess was 0.",Code); ;
Console.ReadKey(true);


3 Answers
3

This is sophisticated for a first attempt, but today would be a great day to break yourself of bad habits. Your code is full of them.

Start with: format your code using standard formatting conventions. We understand code more easily if it is vertical, and your code is horizontal.

Let's go through this line by line.

static void Main(string args)
{


Programs that do everything in Main are programs that cannot be easily refactored or re-used. Main should create an object that represents the program and then execute it.

Random GenRandom = new Random();


Follow C# naming conventions. Locals get camelCase names. This should be

camelCase

Random random = new Random();

int t = 0, r, c1 = random.Next(1, 6), c2 = random.Next(1, 6), c3 = random.Next(1, 6), c4 = random.Next(1, 6);


Though this style is legal, it's considered poor style. Declare and initialize variables one per line. Give your variables meaningful names. You will not die younger if you spend time typing turn instead of t. This should be:

turn

t

int turn = 0;
int digit;
int cell1 = random.Next(1, 6);
int cell2 = random.Next(1, 6);
int cell3 = random.Next(1, 6);
int cell4 = random.Next(1, 6);


Moving on:

bool w = false;


Same deal:

bool hasWon = false;


Moving on:

string Input, Code = Convert.ToString(c1);
Code += c2;
Code += c3;
Code += c4;


Again, one per line.

We now see that we have the same data in two places: the locals c1, c2, c3, c4 which are never used again and Code. It is time for our first refactoring. What we want is:

Code

string code = GenerateCode();


Let's implement that:

static Random random = new Random();
static string GenerateCode()

return random.Next(1, 6).ToString() +
random.Next(1, 6).ToString()
random.Next(1, 6).ToString()
random.Next(1, 6).ToString();



And we remove random, c1, and so on, from our method.

random

c1

All right, where were we? We now have:

string input;
string code = GenerateCode();


Moving on:

while (t != 8) {


First, bad form in the disco brace style. Second, eight? Where did eight come from? Rewrite it:

const int maximumTurns = 8;
while (turn != maximumTurns)
Input.Contains("8")
End:;
Console.Clear();


Again, I'm fine with this but we'll come back to the goto later.

if (w == true) Console.WriteLine("You won! The code you guessed was 0.", Code); else Console.WriteLine("You lost! The code you couldnt guess was 0.",Code); ;


Writing if (w == true) is a sure sign of newbie code. w is already either true or false; you don't have to say "if it is true that w is true" and you don't have to say "if w is true" -- just say "if w".

if (w == true)

w

And again, this could go into a helper.

Now, let's look at your main loop as I have refactored it, all together

static void Main(string args)

int turn = 0;
bool hasWon = false;
string code = GenerateCode();
string input;
const int maximumTurns = 8;
while (turn != maximumTurns)

turn += 1;
Unepic:
PromptUserForInput(maximumTurns, turn);
input = Console.ReadLine();
if (!ValidInput(input)) goto Unepic;
if (input == code)

hasWon = true;
goto End;

WriteHints(input, code);
PromptUserToPressKey();

End:;
PrintWinOrLoseMessage(hasWon, code);



This is already one million times easier to read but we are not done.

The first thing we notice is that we do some work to ensure that turn is not incremented if the user did a bad input. That's great! But the code does not read like that intent. Let's rewrite it so it does.

turn

int turn = 1; // not zero!
while (turn <= maximumTurns) // not !=

Unepic:
PromptUserForInput(maximumTurns, turn);
input = Console.ReadLine();
if (!ValidInput(input)) goto Unepic;
if (input == code)

hasWon = true;
goto End;

WriteHints(input, code);
PromptUserToPressKey();
turn += 1;



Now turn is only ever incremented after the user has put in a good guess, and the code doesn't need to initialize turn to the wrong value so that it can be incremented later.

turn

Now we notice that the goto Unepic and goto End are continue and break:

goto Unepic

goto End

continue

break

while (turn <= maximumTurns)

PromptUserForInput(maximumTurns, turn);
input = Console.ReadLine();
if (!ValidInput(input))
continue;
if (input == code)

hasWon = true;
break;

WriteHints(input, code);
PromptUserToPressKey();
turn += 1;



Look at how much easier to understand my version is compared to yours. Anyone, even a non-coder, could look at this thing and just read it like English. "while the turn is less than or equal to the maximum turns, prompt the user for input, then read the line from the console, then validate the input..."

That is what you must strive for in your code. Always be asking yourself how could I make this easier to understand? How could I make this read more like a description of my intentions?

Could we do better? Sure. We actually have two loops written as one. There's the loop that gets valid user input, and there's the loop that runs the game. Make that explicit:

while (turn <= maximumTurns)

do

PromptUserForInput(maximumTurns, turn);
input = Console.ReadLine();

while(!ValidInput(input));
if (input == code)

hasWon = true;
break;

WriteHints(input, code);
PromptUserToPressKey();
turn += 1;



And now we see another opportunity for helper methods. Move the inner loop to a helper! Move the winning check to a helper:

while (turn <= maximumTurns)

input = ObtainValidatedInput(maximumTurns, turn);
hasWon = CheckForWin(input, code);
if (hasWon)
break;
WriteHints(input, code);
PromptUserToPressKey();
turn += 1;



Were I writing your code from scratch, I would make it considerably more abstract than even the version I've given you here. My main would be:

static void Main()

var game = new Game();
game.Start();
while (!game.IsOver)
game.ExecuteTurn();
game.End();



Try writing your code starting from this template. Notice how clear and logical it forces your code to be when you start from a position of the code reading like an abstract description of the workflow. Make every method like this, where you can read the logical workflow right out of a half dozen lines of code. Get in good habits while you are still a beginner.

Plural

turn

maximumTurns - 1 - turn

remainingTurns

remainingTurns = maximumTurns - 1 - turn; Console.WriteLine($"You have remainingTurns Plural(remainingTurns, "turn") left.");

GenerateCode

+

Random

Don't use meaningless names. GenRandom, t, r, c1,... These don't tell me anything and make your code needlessly obscure.

GenRandom

t

r

c1

This is not a traditional C# coding style:

string Input, Code = Convert.ToString(c1); Code += c2; Code += c3; Code += c4;


The "8" in while (t != 8) and the "9" in Console.WriteLine("You have 0 turn(s) left.",9-t); are likely linked, so I'd expect one of them to be a const with a descriptive name.

while (t != 8)

Console.WriteLine("You have 0 turn(s) left.",9-t);

const

gotos are rarely used in C#. Use methods to separate logic.

goto

Comments should explain why, not what. For instance, // Checks if input is 4 characters long is pointless, since I can see that by reading the code.

// Checks if input is 4 characters long

Both of these lines check the inputted values:

try Convert.ToInt16(Input); Convert.ToString(Input); catch (FormatException) goto Unepic;
if (Input.Contains("0") || Input.Contains("7") || Input.Contains("8") || Input.Contains("9")) goto Unepic;


Why not use a simple Regex?

Between the two lines checking input, you put this line:

if (Input == Code) w = true; goto End; ; // Checks if you've won


This makes no sense to me. You should finish checking the validity of the input before you check whether the inputted value is correct.

goto

goto

Use variable names that are descriptive, so they will help document your code for anyone reading it without needing to add comments.
For example, instead of

bool w = false;


use

bool HasWon = false;


which immediately makes clear that this is concerned with the final winning status. Likewise change "t" to "ValidTries" etc.

From a design viewpoint, if you decide the input is invalid, you should be writing a message to the user to tell them why that is invalid, not just giving them the prompt again.

Instead of the goto commands and the labels for them, use a while loop or a do... while for any time something needs to repeat. Group your two exit conditions (they have won or they have run out of tries) together on the main "keep playing" loop, and then try a do...while loop for looking for a valid try.

while

do... while

do...while

So in pseudocode you want something like:

while (not HasWon and ValidTries < 8)


do

// prompt them to try and read input
// check validity of input and return error message if there is a problem
while (Input is not valid)

HasWon = //check the answer is correct or not
if (not HasWon)

ValidTries ++
// print message that that try is wrong


if (HasWon)
// print winning message saying how many ValidTries it took
else
// print message saying they ran out of tries and giving the correct answer


Required, but never shown

Required, but never shown

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.