24
\$\begingroup\$

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) {
            t++;
        Unepic:
            Console.Clear();
            Console.WriteLine("You have {0} turn(s) left.",9-t);
            Console.WriteLine("Guess the code E.g 1561: ");
            Input = Console.ReadLine();
            if (Input.Length != 4) goto Unepic; // Checks if input is 4 characters long
            try { Convert.ToInt16(Input); Convert.ToString(Input); } catch (FormatException) { goto Unepic; } // Checks if input is only numbers
            if (Input == Code) { w = true; goto End; }; // Checks if you've won
            if (Input.Contains("0") || Input.Contains("7") || Input.Contains("8") || Input.Contains("9")) { goto Unepic; } // Checks if it has any digits that are 0 or above 7
            r = -1;
            while (r != 3)
            {
                r++;
                if (Input[r] == Code[r]) Console.Write(1); else Console.Write(0); // Checks if a digit of the input is equal to a digit of the code
            }
            Console.WriteLine();
            Console.Write("Press any key to continue.");
            Console.ReadKey(true);
        }
        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);
\$\endgroup\$
  • 2
    \$\begingroup\$ What does "unepic" mean? Is that French? \$\endgroup\$ – Tanner Swett Aug 30 '18 at 15:05
  • 18
    \$\begingroup\$ This code is written as though the author believed that they only have so many times to press the return key before they die! :-) Put in line breaks for every statement please. If you feel like a piece of functionality logically is only one line then extract it to a function and call the function, and hey, now it is only one line. \$\endgroup\$ – Eric Lippert Aug 30 '18 at 23:36
53
\$\begingroup\$

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

    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:

    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:

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.

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)
{

Moving on:

  t++;

I find increment operators distasteful.

  turn += 1;

Much nicer.

    Unepic:

OH THE PAIN. Do not write unstructured code in C#; it is 2018, not 1972.

        Console.Clear();

The first line I don't object to! :-) But as we'll see, it should be in a method of its own.

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

Better would be:

        Console.WriteLine($"You have {maximumTurns + 1 - turn} turn(s) left.");

Because now if you change maximumTurns to 7, you only have to change the code in one place, not many.

Better still, make a helper function:

static string Plural(int x, string s)
{
  return x == 1 ? s : s + "s";
} 

and then

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

And then move that into a helper.

Moving on:

        Console.WriteLine("Guess the code E.g 1561: ");

Eschew Latinisms. Should be:

        Console.WriteLine("Guess the code (for example, 1561): ");

Now we come into a section that demands refactoring:

        input = Console.ReadLine();

Fine so far...

        if (input.Length != 4) goto Unepic; // Checks if input is 4 characters long

Do not write comments that repeat what is plainly evident in the code. And do not use spaghetti logic.

        try { Convert.ToInt16(Input); Convert.ToString(Input); } catch (FormatException) { goto Unepic; } // Checks if input is only numbers

Do not re-invent the wheel; TryParse already exists:

    int number; // In C# 7 you can use a discard instead.
    bool valid = int.TryParse(input, out number);

How should we structure this whole thing better? What we want is:

    if (!ValidInput(input)) goto Unepic;

So write that:

static bool ValidateInput(string input)
{
  int number;
  return input.Length == 4 && int.TryParse(input, out number);
}

Moving on:

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

There's a feral semi at the end there. Remove it.

Again, when you are writing comments that are obvious, ask yourself "how could I rewrite the code so that the comment was unnecessary?" Like this:

        if (input == code) 
        { 
          hasWon = true; 
          goto End; 
        }

We'll address the goto later.

        if (Input.Contains("0") || Input.Contains("7") || Input.Contains("8") || Input.Contains("9")) { goto Unepic; } // Checks if it has any digits that are 0 or above 7

Why is this check here? It should be in ValidateInput. Put it there!

We then have a little loop:

        r = -1;
        while (r != 3)
        {
            r++;
            if (Input[r] == Code[r]) Console.Write(1); else Console.Write(0); // Checks if a digit of the input is equal to a digit of the code
        }

Again, put it in a method.

static void WriteHints(string input, string code)
{

The normal way we would write this loop is:

  for (int i = 0; i < input.Length; i += 1) 
    Console.Write(input[i] == code[i] ? 1 : 0;
}

Finishing up the game loop:

        Console.WriteLine();
        Console.Write("Press any key to continue.");
        Console.ReadKey(true);

Why make the user press a key here? But again, it could be put into a helper method.

    }
    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".

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.

    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.

Now we notice that the goto Unepic and goto End are continue and 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.

\$\endgroup\$
  • 24
    \$\begingroup\$ Listen to this man's advice. I would be honored if I got some code review like this from @ericlippert! Very thorough. \$\endgroup\$ – Steve Aug 31 '18 at 2:11
  • 1
    \$\begingroup\$ The call to your factored Plural method is passing turn instead of maximumTurns - 1 - turn, perhaps factoring that out into a remainingTurns variable first would be better e.g. remainingTurns = maximumTurns - 1 - turn; Console.WriteLine($"You have {remainingTurns} {Plural(remainingTurns, "turn")} left."); \$\endgroup\$ – Joshua Webb Aug 31 '18 at 7:43
  • 1
    \$\begingroup\$ Your function GenerateCode is missing a couple of +s \$\endgroup\$ – JAD Aug 31 '18 at 9:52
  • 1
    \$\begingroup\$ Excellent and thorough as always but I think you glossed over why Random should be at the class level and not local to a given method. As presented, it just looks as if it was just for cleaner organization. \$\endgroup\$ – Rick Davin Aug 31 '18 at 13:41
12
\$\begingroup\$

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


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.


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


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.


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.

\$\endgroup\$
  • 8
    \$\begingroup\$ "gotos are rarely used in C#" - I didn't even realize they were part of the language... \$\endgroup\$ – Matthew FitzGerald-Chamberlain Aug 30 '18 at 16:24
  • 1
    \$\begingroup\$ I know it's for PHP but note the appropriate cartoon for goto - php.net/manual/en/control-structures.goto.php \$\endgroup\$ – ggdx Aug 30 '18 at 16:50
  • 3
    \$\begingroup\$ @ggdx The comic was not created for PHP. I reckon it was actually referencing C. Original: xkcd.com/292 \$\endgroup\$ – jkd Aug 30 '18 at 23:44
  • 1
    \$\begingroup\$ @jkd probably, holds true for pretty much any language with this dumb facility though IMO. \$\endgroup\$ – ggdx Aug 30 '18 at 23:46
  • 1
    \$\begingroup\$ @jkd Many computers still do! They just call it VB or VBA, and it's got a lot of convenient, modern language features that go completely ignored because that accountant learned BASIC in the 80s and isn't gonna change his style for some fad like try/catch. \$\endgroup\$ – Nic Hartley Aug 31 '18 at 19:22
2
\$\begingroup\$

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.

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
\$\endgroup\$

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.