24
\$\begingroup\$

I've been teaching myself how to program for a little bit now, and was hoping to get some critique on my most recent project, creating a Tic Tac Toe program! I'm hoping for people to point out some of the flaws within my code, and some areas in which I should try to improve on going forward.

class Program
{
    static string[] pos = new string[10] { "0", "1", "2","3","4","5","6","7","8","9" }; // Array that contains board positions, 0 isnt used --------------------------------

    static void DrawBoard() // Draw board method ==========================================
    {
        Console.WriteLine("   {0}  |  {1}  |  {2}   ", pos[1], pos[2], pos[3]);
        Console.WriteLine("-------------------");
        Console.WriteLine("   {0}  |  {1}  |  {2}   ", pos[4], pos[5], pos[6]);
        Console.WriteLine("-------------------");
        Console.WriteLine("   {0}  |  {1}  |  {2}   ", pos[7], pos[8], pos[9]);
    }

    static void Main(string[] args) // Main ==============================================
    {
        string player1 = "", player2 = "";
        int choice = 0, turn = 1, score1 = 0, score2 = 0;
        bool winFlag = false, playing = true, correctInput = false;

        Console.WriteLine("Hello! This is Tic Tac Toe. If you don't know the rules then stop being an idiot.");
        Console.WriteLine("What is the name of player 1?");
        player1 = Console.ReadLine();
        Console.WriteLine("Very good. What is the name of player 2?");
        player2 = Console.ReadLine();
        Console.WriteLine("Okay good. {0} is O and {1} is X." , player1, player2);
        Console.WriteLine("{0} goes first." , player1);
        Console.ReadLine();
        Console.Clear();

        while (playing == true)
        {
            while (winFlag == false) // Game loop ------------------------------------------------------
            {
                DrawBoard();
                Console.WriteLine("");

                Console.WriteLine("Score: {0} - {1}     {2} - {3}", player1, score1, player2, score2);
                if (turn == 1)
                {
                    Console.WriteLine("{0}'s (O) turn", player1);
                }
                if (turn == 2)
                {
                    Console.WriteLine("{0}'s (X) turn", player2);
                }

                while (correctInput == false)
                {
                    Console.WriteLine("Which position would you like to take?");
                    choice = int.Parse(Console.ReadLine());
                    if (choice > 0 && choice < 10)
                    {
                        correctInput = true;
                    }
                    else
                    {
                        continue;
                    }
                }

                correctInput = false; // Reset -------

                if (turn == 1)
                {
                    if (pos[choice] == "X") // Checks to see if spot is taken already --------------------
                    {
                        Console.WriteLine("You can't steal positions asshole! ");
                        Console.Write("Try again.");
                        Console.ReadLine();
                        Console.Clear();
                        continue;
                    }
                    else
                    {
                        pos[choice] = "O";
                    }
                }
                if (turn == 2)
                {
                    if (pos[choice] == "O") // Checks to see if spot is taken already -------------------
                    {
                        Console.WriteLine("You can't steal positions asshole! ");
                        Console.Write("Try again.");
                        Console.ReadLine();
                        Console.Clear();
                        continue;
                    }
                    else
                    {
                        pos[choice] = "X";
                    }
                }

                winFlag = CheckWin();

                if (winFlag == false)
                {
                    if (turn == 1)
                    {
                        turn = 2;
                    }
                    else if (turn == 2)
                    {
                        turn = 1;
                    }
                    Console.Clear();
                }
            }

            Console.Clear();

            DrawBoard();

            for (int i = 1; i < 10; i++) // Resets board ------------------------
            {
                pos[i] = i.ToString();
            }

            if (winFlag == false) // No one won ---------------------------
            {
                Console.WriteLine("It's a draw!");
                Console.WriteLine("Score: {0} - {1}     {2} - {3}", player1, score1, player2, score2);
                Console.WriteLine("");
                Console.WriteLine("What would you like to do now?");
                Console.WriteLine("1. Play again");
                Console.WriteLine("2. Leave");
                Console.WriteLine("");

                while (correctInput == false)
                {
                    Console.WriteLine("Enter your option: ");
                    choice = int.Parse(Console.ReadLine());

                    if (choice > 0 && choice < 3)
                    {
                        correctInput = true;
                    }
                }

                correctInput = false; // Reset -------------

                switch (choice)
                {
                    case 1:
                        break;
                    case 2:
                        Console.Clear();
                        Console.WriteLine("Thanks for playing!");
                        Console.ReadLine();
                        playing = false;
                        break;
                }
            }

            if (winFlag == true) // Someone won -----------------------------
            {
                if(turn == 1)
                {
                    score1++;
                    Console.WriteLine("{0} wins!" , player1);
                    Console.WriteLine("What would you like to do now?");
                    Console.WriteLine("1. Play again");
                    Console.WriteLine("2. Leave");

                    while (correctInput == false)
                    {
                        Console.WriteLine("Enter your option: ");
                        choice = int.Parse(Console.ReadLine());

                        if (choice > 0 && choice < 3)
                        {
                            correctInput = true;
                        }
                    }

                    correctInput = false; // Reset --------------

                    switch (choice)
                    {
                        case 1:
                            Console.Clear();
                            winFlag = false;
                            break;
                        case 2:
                            Console.Clear();
                            Console.WriteLine("Thanks for playing!");
                            Console.ReadLine();
                            playing = false;
                            break;
                    }
                }

                if (turn == 2)
                {
                    score2++;
                    Console.WriteLine("{0} wins!" , player2);
                    Console.WriteLine("What would you like to do now?");
                    Console.WriteLine("1. Play again");
                    Console.WriteLine("2. Leave");

                    while (correctInput == false)
                    {
                        Console.WriteLine("Enter your option: ");
                        choice = int.Parse(Console.ReadLine());

                        if (choice > 0 && choice < 3)
                        {
                            correctInput = true;
                        }
                    }

                    correctInput = false; // Reset -----------------

                    switch (choice)
                    {
                        case 1:
                            Console.Clear();
                            winFlag = false;
                            break;
                        case 2:
                            Console.Clear();
                            Console.WriteLine("Thanks for playing!");
                            Console.ReadLine();
                            playing = false;
                            break;
                    }
                }
            }
        }
    }

    static bool CheckWin() // Win checker method ================================================
    {
        if (pos[1] == "O" && pos[2] == "O" && pos[3] == "O") // Horizontal ----------------------------------------
        {
            return true;
        }
        else if (pos[4] == "O" && pos[5] == "O" && pos[6] == "O")
        {
            return true;
        }
        else if(pos[7] == "O" && pos[8] == "O" && pos[9] == "O")
        {
            return true;
        }

        else if(pos[1] == "O" && pos[5] == "O" && pos[9] == "O") // Diagonal -----------------------------------------
        {
            return true;
        }
        else if(pos[7] == "O" && pos[5] == "O" && pos[3] == "O")
        {
            return true;
        }

        else if(pos[1] == "O" && pos[4] == "O" && pos[7] == "O")// Coloumns ------------------------------------------
        {
            return true;
        }
        else if(pos[2] == "O" && pos[5] == "O" && pos[8] == "O")
        {
            return true;
        }
        else if(pos[3] == "O" && pos[6] == "O" && pos[9] == "O")
        {
            return true;
        }

        if (pos[1] == "X" && pos[2] == "X" && pos[3] == "X") // Horizontal ----------------------------------------
        {
            return true;
        }
        else if (pos[4] == "X" && pos[5] == "X" && pos[6] == "X")
        {
            return true;
        }
        else if (pos[7] == "X" && pos[8] == "X" && pos[9] == "X")
        {
            return true;
        }

        else if (pos[1] == "X" && pos[5] == "X" && pos[9] == "X") // Diagonal -----------------------------------------
        {
            return true;
        }
        else if (pos[7] == "X" && pos[5] == "X" && pos[3] == "X")
        {
            return true;
        }

        else if (pos[1] == "X" && pos[4] == "X" && pos[7] == "X") // Coloumns ------------------------------------------
        {
            return true;
        }
        else if (pos[2] == "X" && pos[5] == "X" && pos[8] == "X")
        {
            return true;
        }
        else if (pos[3] == "X" && pos[6] == "X" && pos[9] == "X")
        {
            return true;
        }
        else // No winner ----------------------------------------------
        {
            return false;
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ "// No one won" is unreachable code. \$\endgroup\$ – Nigel Thorne Aug 11 '17 at 9:40
9
\$\begingroup\$

OK .. I took a look at your code. Let's do this one step at a time...

I'm using VisualStudio with Resharper Installed. This causes a big chunk of your code to grey out.

The if clause

if (winFlag == false) // No one won ---------------------------
{
  ...
}

This is because the winFlag can't be false. This because the loop above only exits when the winFlag is true.

So... there is a problem in there. We will get to that. Let's do some cleanup.

CheckWin is a good place to start. Lots of verbosity. The need to add comments shows the method doesn't speak by itself.

    if(Pos[1] == "O" && Pos[5] == "O" && Pos[9] == "O") // Diagonal -----------------------------------------
    {
        return true;
    }

This pattern repeats... what is it doing? Checking for a line. Lets add a method for that.

    private static bool IsLine(int index0, int index1, int index2, string piece)
    {
        return Pos[index0] == piece && Pos[index1] == piece && Pos[index2] == piece;
    }

Applying that function we get ...

static bool CheckWin() // Win checker method ================================================
    {
        if (IsLine(1, 2, 3, "O")) // Horizontal ----------------------------------------
        {
            return true;
        }
        if (IsLine(4, 5, 6, "O"))
        {
            return true;
        }
        if (IsLine(7, 8, 9, "O"))
        {
            return true;
        }

        if(IsLine(1, 5, 9, "O")) // Diagonal -----------------------------------------
        {
            return true;
        }
        if(IsLine(7, 5, 3, "O"))
        {
            return true;
        }

        if(IsLine(1, 4, 7, "O"))// Coloumns ------------------------------------------
        {
            return true;
        }
        if(IsLine(2, 5, 8, "O"))
        {
            return true;
        }
        if(IsLine(3, 6, 9, "O"))
        {
            return true;
        }

        if(IsLine(1, 2, 3, "X")) // Horizontal ----------------------------------------
        {
            return true;
        }
        if(IsLine(4, 5, 6, "X"))
        {
            return true;
        }
        if(IsLine(7, 8, 9, "X"))
        {
            return true;
        }

        if(IsLine(1, 5, 9, "X")) // Diagonal -----------------------------------------
        {
            return true;
        }
        if(IsLine(7, 5, 3, "X"))
        {
            return true;
        }

        if(IsLine(1, 4, 7, "X")) // Coloumns ------------------------------------------
        {
            return true;
        }
        if(IsLine(2, 5, 8, "X"))
        {
            return true;
        }
        if(IsLine(3, 6, 9, "X"))
        {
            return true;
        }
        return false;
    }

Okay, nice, but there is another chunk of duplication. All the tests get done twice, once with 'O' and once with 'X'. Lets deal with that.... now we can do this in a couple of ways. Loop over possible pieces, or just change the IsLine to check all the pieces in the line match.

Lets try the second.

    private static bool IsAnyLine(int index0, int index1, int index2)
    {
        return IsLine(index0, index1, index2, Pos[index0]);
    }

And we get:

    static bool CheckWin() // Win checker method ================================================
    {
        if(IsAnyLine(1, 2, 3)) // Horizontal ----------------------------------------
        {
            return true;
        }
        if(IsAnyLine(4, 5, 6))
        {
            return true;
        }
        if(IsAnyLine(7, 8, 9))
        {
            return true;
        }

        if(IsAnyLine(1, 5, 9)) // Diagonal -----------------------------------------
        {
            return true;
        }
        if(IsAnyLine(7, 5, 3))
        {
            return true;
        }

        if(IsAnyLine(1, 4, 7))// Coloumns ------------------------------------------
        {
            return true;
        }
        if(IsAnyLine(2, 5, 8))
        {
            return true;
        }
        if(IsAnyLine(3, 6, 9))
        {
            return true;
        }

        return false;
    }

Now.... what about all those "return true" exits from this method. We just need to return true if any of those cases are true.

static bool CheckWin() // Win checker method ================================================
{
    if (IsAnyLine(1, 2, 3) ||
        IsAnyLine(4, 5, 6) ||
        IsAnyLine(7, 8, 9) ||
        IsAnyLine(1, 5, 9) ||
        IsAnyLine(7, 5, 3) ||
        IsAnyLine(1, 4, 7) ||
        IsAnyLine(2, 5, 8) ||
        IsAnyLine(3, 6, 9)) return true;
    return false;
}

Oh ... "if X==true return true else return false" is the same as "return X"

    static bool CheckWin() // Win checker method ================================================
    {
        return IsAnyLine(1, 2, 3) || // Horizontal 
               IsAnyLine(4, 5, 6) || // Horizontal 
               IsAnyLine(7, 8, 9) || // Horizontal 
               IsAnyLine(1, 5, 9) || // Diagonal
               IsAnyLine(7, 5, 3) || // Diagonal
               IsAnyLine(1, 4, 7) || // Vertical
               IsAnyLine(2, 5, 8) || // Vertical
               IsAnyLine(3, 6, 9);   // Vertical
    }

Now... Interestingly Horizontal lines are all (x,x+1,x+2) and all vertical are (x,x+3,x+6). One diagonal is "(x, x+4, x+8)" the other is (3, 3+2, 3+4)

    private static bool IsAnyHorizontalLine(int startindex)
    {
        return IsAnyLine(startindex, startindex + 1, startindex + 2);
    }

    private static bool IsAnyVerticalLine(int startindex)
    {
        return IsAnyLine(startindex, startindex + 3, startindex + 6);
    }

    static bool CheckWin() // Win checker method ================================================
    {
        return IsAnyHorizontalLine(1) ||
               IsAnyHorizontalLine(4) ||
               IsAnyHorizontalLine(7) ||
               IsAnyLine(1, 5, 9) ||
               IsAnyLine(7, 5, 3) ||
               IsAnyVerticalLine(1) ||
               IsAnyVerticalLine(2) ||
               IsAnyVerticalLine(3);
    }

One more thing...

    private static bool IsAnyLine(int start, int step)
    {
        return IsLine(start, start+step, start+step+step, Pos[start]);
    }

    private static bool IsAnyHorizontalLine(int startindex)
    {
        return IsAnyLine(startindex, 1);
    }

    private static bool IsAnyVerticalLine(int startindex)
    {
        return IsAnyLine(startindex, 3);
    }

    static bool CheckWin() // Win checker method ================================================
    {
        return IsAnyHorizontalLine(1) ||
               IsAnyHorizontalLine(4) ||
               IsAnyHorizontalLine(7) ||
               IsAnyLine(1, 4) ||  // Diagonal 
               IsAnyLine(3, 2) ||  // Diagonal
               IsAnyVerticalLine(1) ||
               IsAnyVerticalLine(2) ||
               IsAnyVerticalLine(3);
    }

OK we could do more here... but I think this is enough.

Let's look at the main logic.

code_overview

Now that is one big function.

Lets break out some helper functions... Every time you have a comments I'll make it a function. Same with the body of a loop.. make it a function.

    private static string[] EnterPlayers()
    {
        Console.WriteLine("Hello! This is Tic Tac Toe. If you don't know the rules then stop being an idiot.");
        Console.WriteLine("What is the name of player 1?");
        var player1 = Console.ReadLine();
        Console.WriteLine("Very good. What is the name of player 2?");
        var player2 = Console.ReadLine();
        Console.WriteLine("Okay good. {0} is O and {1} is X.", player1, player2);
        Console.WriteLine("{0} goes first.", player1);
        Console.ReadLine();
        Console.Clear();
        return new [] {player1, player2};
    }

That makes the first few lines...

        string[] players = EnterPlayers();
        string player1 = players[0];
        string player2 = players[1];

Nice. I wonder if a player array is useful? maybe... but lets limit the changes for now and keep using the player variables.

In C++ it was good practice to have all your variables declared at the start of the method. In C# we move them to where they are first used. Try for the smallest scope you can for each variable. With that in mind it's also a bad idea to reuse variables. Better to make a new one each time you need it. This decouples code that shouldn't be coupled. This makes changing your code easier.

The 'choice' variable is reused each time we ask the user for data. Same with 'correctInput'.

while (correctInput == false)
{
     Console.WriteLine("Which position would you like to take?");
     choice = int.Parse(Console.ReadLine());
     if (choice > 0 && choice < 10)
     {
         correctInput = true;
     }
 }

It's always the same code, so let's make a function.

private static int AskTheUser(string prompt)
{
    bool correctInput = false;
    int choice = 0;
    while (correctInput == false)
    {
        Console.WriteLine(prompt);
        choice = int.Parse(Console.ReadLine());

        if (choice > 0 && choice < 3)
        {
            correctInput = true;
        }
    }
    return choice;
}

Or simpler...

private static int AskTheUser(string prompt, int min, int max)
{
    while (true)
    {
        Console.WriteLine(prompt);
        int choice = int.Parse(Console.ReadLine());

        if (choice >= min && choice <= max)
        {
            return choice;
        }
    }
}
... 
var choice = AskTheUser("Enter your option: ", 1, 2);

The method is still too big to read... lets pull out "Checks to see if spot is taken already"

    private static bool TryToPlacePiece(int move, string opponentsPiece, string piece)
    {
        if (Pos[move] == opponentsPiece) // Checks to see if spot is taken already --------------------
        {
            Console.WriteLine("You can't steal positions asshole! ");
            Console.Write("Try again.");
            Console.ReadLine();
            Console.Clear();
            return true;
        }
        Pos[move] = piece;
        return false;
    }

"What would you like to do now?"

    private static bool AskToPlayAgain()
    {
        Console.WriteLine("What would you like to do now?");
        Console.WriteLine("1. Play again");
        Console.WriteLine("2. Leave");

        var choice = AskTheUser("Enter your option: ", 1, 2);

        Console.Clear();
        if (choice == 1) return true;

        Console.WriteLine("Thanks for playing!");
        Console.ReadLine();
        return false;
    }

I did do some code manipulation here... but now it's a method that returns true to keep playing and false otherwise.

Almost manageable

Turn1and2

There are two turns and different logic for each. If we extract a method for the loop, we can see what parameters it needs

            while (winFlag == false) // Game loop ------------------------------------------------------
            {
                if (TakeTurn(player1, score1, player2, score2, turn)) continue;

                winFlag = CheckWin();

                if (winFlag == false)
                {
                    if (turn == 1)
                    {
                        turn = 2;
                    }
                    else if (turn == 2)
                    {
                        turn = 1;
                    }
                    Console.Clear();
                }
            }

Lets look at "TakeTurn"...

    private static bool TakeTurn(string player1, int score1, string player2, int score2, int turn)
    {
        DrawBoard();
        Console.WriteLine("");

        Console.WriteLine("Score: {0} - {1}     {2} - {3}", player1, score1, player2, score2);
        if (turn == 1)
        {
            Console.WriteLine("{0}'s (O) turn", player1);
        }
        if (turn == 2)
        {
            Console.WriteLine("{0}'s (X) turn", player2);
        }

        var move = AskTheUser("Which position would you like to take?", 1, 9);

        if (turn == 1)
        {
            if (TryToPlacePiece(move, "X", "O")) return true;
        }
        if (turn == 2)
        {
            if (TryToPlacePiece(move, "O", "X")) return true;
        }
        return false;
    }

The only times 'score1' and 'score2' are used is the first few lines. Lets pull those out

    private static void RefreshBoard(string player1, int score1, string player2, int score2)
    {
        DrawBoard();
        Console.WriteLine("");

        Console.WriteLine("Score: {0} - {1}     {2} - {3}", player1, score1, player2, score2);
    }

Right.. lets group the 'turn==1' logic.

    private static bool TakeTurn(string player1, string player2, int turn)
    {
        if (turn == 1)
        {
            Console.WriteLine("{0}'s ({1}) turn", player1, "O");
            var move = AskTheUser("Which position would you like to take?", 1, 9);
            if (TryToPlacePiece(move, "X", "O")) return true;
        }
        else
        {
            Console.WriteLine("{0}'s ({1}) turn", player2, "X");
            var move = AskTheUser("Which position would you like to take?", 1, 9);
            if (TryToPlacePiece(move, "O", "X")) return true;
        }
        return false;
    }

Pull out the method

    private static bool TakeATurn(string player, string playerPiece, string opponentsPiece)
    {
        Console.WriteLine("{0}'s ({1}) turn", player, playerPiece);
        var move = AskTheUser("Which position would you like to take?", 1, 9);
        return TryToPlacePiece(move, opponentsPiece, playerPiece);
    }

This seems a lot like it should be inside TryToPlacePiece.

So we get...

private static bool TryToTakeATurn(string player1, string player2, int turn)
{
    if (turn == 1)
    {
        if (TryToPlaceAPiece(player1, "O", "X")) return true;
    }
    else
    {
        if (TryToPlaceAPiece(player2, "X", "O")) return true;
    }
    return false;
}

private static bool TryToPlaceAPiece(string player, string playerPiece, string opponentsPiece)
{
    Console.WriteLine("{0}'s ({1}) turn", player, playerPiece);
    var move = AskTheUser("Which position would you like to take?", 1, 9);
    if (Pos[move] == opponentsPiece) // Checks to see if spot is taken already --------------------
    {
        Console.WriteLine("You can't steal positions asshole! ");
        Console.Write("Try again.");
        Console.ReadLine();
        Console.Clear();
        return true;
    }
    Pos[move] = playerPiece;
    return false;
}

Now Main is readable... kinda :)

public static void Main(string[] args)
    {
        string[] players = EnterPlayers();
        string player1 = players[0];
        string player2 = players[1];

        bool playing = true;
        int score1 = 0, score2 = 0;
        while (playing)
        {
            bool winFlag = false;
            int turn = 1;
            while (winFlag == false) // Game loop ------------------------------------------------------
            {
                RefreshBoard(player1, score1, player2, score2);
                if (TryToTakeATurn(player1, player2, turn)) continue;

                winFlag = CheckWin();

                if (winFlag == false)
                {
                    turn = turn == 1 ? 2 : 1;
                    Console.Clear();
                }
            }

            Console.Clear();

            DrawBoard();

            ResetBoard();

            if (winFlag == false) // No one won ---------------------------
            {
                Console.WriteLine("It's a draw!");
                Console.WriteLine("Score: {0} - {1}     {2} - {3}", player1, score1, player2, score2);
                Console.WriteLine("");
                playing = AskToPlayAgain();
            }

            if (winFlag) // Someone won -----------------------------
            {
                if (turn == 1)
                {
                    score1++;
                    Console.WriteLine("{0} wins!", player1);
                    playing = AskToPlayAgain();
                }
                if (turn == 2)
                {
                    score2++;
                    Console.WriteLine("{0} wins!", player2);
                    playing = AskToPlayAgain();
                }
            }
        }
    }

At this point it's obvious why we are having problem with WinFlag.

'CheckWin' is setting the 'winFlag'. But CheckWin doesn't detect a draw. A draw is reached when there are no more places to put a piece. (or after 9 moves)

'TryToPlaceAPiece' also has a problem. It stops you putting a piece on-top of an opponents piece, but not on-top of your own.

The biggest smell left if this 'turn' variable. A lot of things are using it as their indicator as to which player is active. Why does the rest of the code care which player is active? The logic should be the same for both. Lets group together all the player specific logic. I'll start by moving it all out of the main method and into helper methods.

We can use the players array, and I'll make a score array and put the pieces into an array... then we get

    private static void IncrementPlayerScore(int turn, int[] scores)
    {
        scores[turn-1] = scores[turn - 1]+1;
    }

    private static void ReportScores(string[] players, int[] scores)
    {
        Console.WriteLine("Score: {0} - {1}     {2} - {3}", players[0], scores[0], players[1], scores[1]);
    }

    private static string GetPlayer(int turn, string[] players)
    {
        return players[turn - 1];
    }

    private static string GetPlayerPiece(int turn, string[] pieces)
    {
        return pieces[turn - 1];
    }

    private static int NextTurn(int turn)
    {
        return 3 - turn;
    }

These helpful methods.

OK This is getting way too long... lets jump some steps.

    public static void Main(string[] args)
    {
        string[] players = EnterPlayers();
        string[] pieces = {"O", "X"};
        int[] scores = {0, 0};

        bool playing = true;
        while (playing)
        {
            PlayGame(players, scores, pieces);
            playing = AskToPlayAgain();
        }
    }

This is what Main looks like now. We are using 'turn-1' as a player index in the player array, scores array and pieces array. We might as well change turn to be 0 based, to remove the logic.

So... here is the final code... in total

class Program
{
    // Array that contains board positions, 0 isnt used --------------------------------
    private static readonly string[] Pos = { "0", "1", "2","3","4","5","6","7","8","9" };

    private static void DrawBoard() // Draw board method ==========================================
    {
        Console.WriteLine("   {0}  |  {1}  |  {2}   ", Pos[1], Pos[2], Pos[3]);
        Console.WriteLine("-------------------");
        Console.WriteLine("   {0}  |  {1}  |  {2}   ", Pos[4], Pos[5], Pos[6]);
        Console.WriteLine("-------------------");
        Console.WriteLine("   {0}  |  {1}  |  {2}   ", Pos[7], Pos[8], Pos[9]);
    }

    private static string[] EnterPlayers()
    {
        Console.WriteLine("Hello! This is Tic Tac Toe. If you don't know the rules then stop being an idiot.");
        Console.WriteLine("What is the name of player 1?");
        var player1 = Console.ReadLine();
        Console.WriteLine("Very good. What is the name of player 2?");
        var player2 = Console.ReadLine();
        Console.WriteLine("Okay good. {0} is O and {1} is X.", player1, player2);
        Console.WriteLine("{0} goes first.", player1);
        Console.ReadLine();
        Console.Clear();
        return new [] {player1, player2};
    }

    public static void Main(string[] args)
    {
        string[] players = EnterPlayers();
        string[] pieces = {"O", "X"};
        int[] scores = {0, 0};

        bool playing = true;
        while (playing)
        {
            PlayGame(players, scores, pieces);
            playing = AskToPlayAgain();
        }
    }

    private static void PlayGame(string[] players, int[] scores, string[] pieces)
    {
        bool isGameWon = false;
        bool isGameOver = false;
        int playerIndex = 0;

        while (isGameOver == false)
        {
            var nextPlayerIndex = 1 - playerIndex;
            var player = players[playerIndex];
            var piece = pieces[playerIndex];
            var opponentPiece = pieces[nextPlayerIndex];

            PlayerMakesAMove(players, scores, player, piece, opponentPiece);

            isGameWon = CheckWin();
            isGameOver = isGameWon || CheckDraw();

            if (isGameOver == false)
            {
                playerIndex = nextPlayerIndex;
            }
        }

        Console.Clear();
        DrawBoard();
        ResetBoard();

        if (isGameWon) // Someone won -----------------------------
        {
            IncrementPlayerScore(scores, playerIndex);
            Console.WriteLine("{0} wins!", players[playerIndex]);
        }
        else // No one won ---------------------------
        {
            Console.WriteLine("It's a draw!");
        }

        ReportScores(players, scores);
    }

    private static void PlayerMakesAMove(string[] players, int[] scores, string player, string piece, string opponentPiece)
    {
        do
        {
            Console.Clear();
            DrawBoard();
            Console.WriteLine("");
            ReportScores(players, scores);
        } while (!TryToPlaceAPiece(player, piece, opponentPiece));
    }

    private static bool CheckDraw()
    {
        // TODO
        return false;
    }

    private static void IncrementPlayerScore(int[] scores, int playerIndex)
    {
        scores[playerIndex] = scores[playerIndex] + 1;
    }

    private static void ReportScores(string[] players, int[] scores)
    {
        Console.WriteLine("Score: {0} - {1}     {2} - {3}", players[0], scores[0], players[1], scores[1]);
    }

    private static bool TryToPlaceAPiece(string player, string playerPiece, string opponentsPiece)
    {
        Console.WriteLine("{0}'s ({1}) turn", player, playerPiece);
        var move = AskTheUser("Which position would you like to take?", 1, 9);
        if (!IsMoveTaken(playerPiece, opponentsPiece, move))
        {
            Pos[move] = playerPiece;
            return true;
        }

        Console.WriteLine("That positions is taken dude! ");
        Console.Write("Try again.");
        Console.ReadLine();
        Console.Clear();
        return false;
    }

    private static bool IsMoveTaken(string playerPiece, string opponentsPiece, int move)
    {
        return Pos[move] == opponentsPiece || Pos[move] == playerPiece;
    }

    private static bool AskToPlayAgain()
    {
        Console.WriteLine("");
        Console.WriteLine("What would you like to do now?");
        Console.WriteLine("1. Play again");
        Console.WriteLine("2. Leave");

        var choice = AskTheUser("Enter your option: ", 1, 2);

        Console.Clear();
        if (choice == 1) return true;

        Console.WriteLine("Thanks for playing!");
        Console.ReadLine();
        return false;
    }

    private static int AskTheUser(string prompt, int min, int max)
    {
        while (true)
        {
            Console.WriteLine(prompt);
            int choice = int.Parse(Console.ReadLine());

            if (choice >= min && choice <= max)
            {
                return choice;
            }
        }
    }

    private static void ResetBoard()
    {
        for (int i = 1; i < 10; i++)
        {
            Pos[i] = i.ToString();
        }
    }

    static bool CheckWin() // Win checker method ================================================
    {
        return IsAnyHorizontalLine(1) ||
               IsAnyHorizontalLine(4) ||
               IsAnyHorizontalLine(7) ||
               IsAnyLine(1, 4) ||  // Diagonal
               IsAnyLine(3, 2) ||  // Diagonal
               IsAnyVerticalLine(1) ||
               IsAnyVerticalLine(2) ||
               IsAnyVerticalLine(3);
    }

    private static bool IsLine(int index0, int index1, int index2, string piece)
    {
        return Pos[index0] == piece && Pos[index1] == piece && Pos[index2] == piece;
    }

    private static bool IsAnyLine(int start, int step)
    {
        return IsLine(start, start+step, start+step+step, Pos[start]);
    }

    private static bool IsAnyHorizontalLine(int startindex)
    {
        return IsAnyLine(startindex, 1);
    }

    private static bool IsAnyVerticalLine(int startindex)
    {
        return IsAnyLine(startindex, 3);
    }
}

The goal here was to make the code more readable. You want to go from "not obviously wrong" to "Obviously not wrong". Having small methods that show your intent and encapsulate your implementation decisions does this for you.

So.... using the squint test again... The biggest method is somewhat manageable. Final State

Taking this further...

C# is Object Oriented. You can use these features to separate the code into blocks each with it's own single responsibility.

Some examples: a Board Object that knows how to draw itself and detect lines. A Player object that knows it's score, name and what piece it is using. A Game object that knows the flow of a game. A Tournament object that knows how to start new games if people want to keep playing.

We can give that a go if you want more feedback.

Good luck!

\$\endgroup\$
  • \$\begingroup\$ Wow , thank you for such a robust answer. Very insightful ! I'll try it out . \$\endgroup\$ – Yummy275 Aug 12 '17 at 2:14
  • \$\begingroup\$ Its good feedback with one big caveat. You didnt utilize any OOP principals. no object for player, for the game, for the state.. nothing \$\endgroup\$ – downrep_nation Aug 13 '17 at 13:26
  • 2
    \$\begingroup\$ @downrep_nation See the last 10 lines of my answer :) \$\endgroup\$ – Nigel Thorne Aug 13 '17 at 13:46
14
\$\begingroup\$

Bug

The same player can place its sign on a allready taken (by him) place. You have defended against this for the opponent although you use inappropriate language here.

You should just check if the place in the grid is allready taken without checking who is currently playing.

I would suggest using a method for this like

private static bool IsPositionTaken(int choice)
{
    return pos[choice] == "X" || pos[choice] == "O";
}  

now you can do the check by

if (turn == 1)
{
    if (IsPositionTaken(choice)) 
    {
        Console.WriteLine("This position is allready taken! ");
        Console.Write("Try again.");
        Console.ReadLine();
        Console.Clear();
        continue;
    }
    else
    {
        pos[choice] = "O";
    }
}

or if you would introduce a small dictionary containing the O and X like

private static Dictionary<int, string> signs = new Dictionary<int, string>
{
    {1, "O"},
    {2, "X"}
};  

then this

if (turn == 1)
{
    if (pos[choice] == "X") // Checks to see if spot is taken already --------------------
    {
        Console.WriteLine("You can't steal positions asshole! ");
        Console.Write("Try again.");
        Console.ReadLine();
        Console.Clear();
        continue;
    }
    else
    {
        pos[choice] = "O";
    }
}
if (turn == 2)
{
    if (pos[choice] == "O") // Checks to see if spot is taken already -------------------
    {
        Console.WriteLine("You can't steal positions asshole! ");
        Console.Write("Try again.");
        Console.ReadLine();
        Console.Clear();
        continue;
    }
    else
    {
        pos[choice] = "X";
    }
}  

would become this

if (IsPositionTaken(choice)) 
{
    Console.WriteLine("This position is allready taken! ");
    Console.Write("Try again.");
    Console.ReadLine();
    Console.Clear();
    continue;
}
else
{
    pos[choice] = signs[turn];
}
\$\endgroup\$
10
\$\begingroup\$

First of all, why pos has 10 elements and everywhere you counting from 1 to 9? Let this array to have 9 elements and count from 0.


You must rewrite the CheckWin method.

Define a list of all winning positions:

// List of 3-elements arrays that define indices
// of winning lines (horizontal, vertical, diagonal)
private static readonly int[][] _winningPositions = new[]
{
    // Horizontal lines

    new[] { 0, 1, 2 },
    new[] { 3, 4, 5 },
    new[] { 6, 7, 8 },

    // Vertical lines

    new[] { 0, 3, 6 },
    new[] { 1, 4, 7 },
    new[] { 2, 5, 8 },

    // Diagonal lines

    new[] { 0, 4, 8 },
    new[] { 2, 4, 6 }
}

Then CheckWin will be (using LINQ and lambdas)

static bool CheckWin()
{
    // Check whether there is a line where all symbols are equal
    return _winningPositions.Any(p =>
           {
               var values = p.Select(i => pos[i]) // get symbols on a line
                             .Distinct()          // get unique symbols on a line
                             .ToArray();
               return values.Length == 1 // all symbols are equal
                                         // so count of unique ones is 1
                      && !string.IsNullOrEmpty(values[0]);
           });
}

You don't need to write boolean literals in conditions. Instead of

while (playing == true)
{
    while (winFlag == false)

write

while (playing)
{
    while (!winFlag)
\$\endgroup\$
  • 3
    \$\begingroup\$ Perhaps a better explanation of what the Lambda is doing? This has been tagged as Beginner so they may not have ever encountered Lambda before. \$\endgroup\$ – Imperial Justinian Aug 4 '17 at 10:33
  • 1
    \$\begingroup\$ When I was a beginner I just was googling any unknown constructions. I think a little research by yourself is almost always better. Usually if some thing explained enough to complete a task there is no motivation to go deeper :) Also I assume if a user asks a question here he is already know the basics of programming language. \$\endgroup\$ – Maxim Aug 4 '17 at 10:43
  • 2
    \$\begingroup\$ @Maxim It's probably still helpful to at least mention the term lambda (although I was slightly surprised that the top two results for Googling just => were JavaScript "arrow" functions and C# lambdas -- I was expecting it to be harder to search for the construct). \$\endgroup\$ – TripeHound Aug 4 '17 at 11:38
  • 1
    \$\begingroup\$ OK, I've added some comments :) \$\endgroup\$ – Maxim Aug 4 '17 at 12:15
  • 1
    \$\begingroup\$ +1 to Explanation of the lambda. I've used LINQ before (complete C# noob though) and I would have no idea of where to start on re-using that bit of code. Everything else makes perfect sense though :). While doing it all for someone can reduce their willingness to do the research, I also tend to find that the more motivated programmers will understand the first example (if well explained) and use that as a reason to learn more in-depth. \$\endgroup\$ – Brandon Barney Aug 4 '17 at 17:37
5
\$\begingroup\$

Your "DrawBoard" method could be improved.

static void DrawBoard() // Draw board method ==========================================
{
    Console.WriteLine("   {0}  |  {1}  |  {2}   ", pos[1], pos[2], pos[3]);
    Console.WriteLine("-------------------");
    Console.WriteLine("   {0}  |  {1}  |  {2}   ", pos[4], pos[5], pos[6]);
    Console.WriteLine("-------------------");
    Console.WriteLine("   {0}  |  {1}  |  {2}   ", pos[7], pos[8], pos[9]);
}

Both " {0} | {1} | {2} " and "-------------------" is repeated multiple times, so they can and should be stored in a variable to be more manageable. I understand this is a small project, but your current usage is a bad habit.

static void DrawBoard() // Draw board method ==========================================
{
    string format = "   {0}  |  {1}  |  {2}   ";
    string separator = "-------------------"
    Console.WriteLine(format , pos[1], pos[2], pos[3]);
    Console.WriteLine(separator);
    Console.WriteLine(format , pos[4], pos[5], pos[6]);
    Console.WriteLine(separator);
    Console.WriteLine(format , pos[7], pos[8], pos[9]);
}

With this change, if you want to change the look of your board (For example add a "+" in the separator when it lines up with a "|", you only need to do it once.

Now, with this new code you can also clearly see that there's a pattern between format and separator in your DrawBoard method. You could make this more flexible by using a For loop to create the board.

static void DrawBoard() // Draw board method ==========================================
{
    string format = "   {0}  |  {1}  |  {2}   ";
    string separator= "-------------------"
    int boardSize = 9;
    int Columns = 3;

    for (int i = 1; i < boardSize; i+=Columns;) { // Pos will go from 1 to 4, then 4 to 7
        Console.WriteLine(format , pos[i], pos[i+1], pos[i+2]);
        Console.WriteLine(separator);
    }
}

While the previous state looks nice for your current situation, the values i+X are hard coded and not flexible. To remedy this, you can send an Array to the WriteLine function to take parameters.

static void DrawBoard() // Draw board method ==========================================
{
    string format = "   {0}  |  {1}  |  {2}   ";
    string separator= "-------------------"
    int boardSize = 9;
    int Columns = 3;

    for (int i = 1; i < boardSize; i+=Columns;) {            
        Console.WriteLine(format, pos.Skip(i).ToArray()); // Skip to position i (1, 4, 7 in this case)
        Console.WriteLine(separator);
    }
}

We're almost done! As you have probably noticed, the format can be automated.

static void DrawBoard() // Draw board method ==========================================
{
    string format = "   {0}  "; // Start with {0} to simplify the rest of the formatting. You don't want a column separator before the {0} or at the end of your board.
    string columnSeparator = "  |  ";
    string rowSeparator= ""
    int Columns = 3;
    int Rows = 3;
    int boardSize = Rows * Columns;

    for (int i = 1; i < Columns; i++) {
        format += columnSeparator + "{" + i + "}"; // Dynamically build your format string based on the number of columns.
    }

    for (int i = 0; i < format.Length; i++) {
        rowSeparator += "-"; // Build the separator dynamically. Don't need to manually count the number of hyphens or guess how many you need!
    }

    for (int i = 1; i < boardSize; i+=Columns;) {            
        Console.WriteLine(format, pos.Skip(i).ToArray()); // Skip to position i (1, 4, 7 in this case)
        Console.WriteLine(rowSeparator);
    }
}

With these changes, you can probably see that your board could scale to a different format, such as a 5x5 sized board if you chose to do so with minimal changes. This added flexibility could prove to be useful in different scenarios, or if you decide to add different board formats. It is a much easier change and less prone human errors when you modify it.

\$\endgroup\$
  • \$\begingroup\$ To clean up the pos + X values you would just need a second for loop, and it would then very easily be able to be scaled up (perhaps even to allow the user to define board size X * Y. The few lines of code it would add would be well worth it IMO :). \$\endgroup\$ – Brandon Barney Aug 4 '17 at 17:41
  • \$\begingroup\$ Indeed, a few lines of code. But I did not deem this necessary for the purpose of reworking this function as it was not what I was looking to demonstrate initially. However, I will add it for completion's sake. \$\endgroup\$ – Yousend Aug 4 '17 at 17:57
  • \$\begingroup\$ @BrandonBarney A loop to create the board would be nice. That way the amount of columns would dictate the format. \$\endgroup\$ – Yousend Aug 4 '17 at 18:08
  • \$\begingroup\$ In my mind (coming from VBA) I would do either a Square board (loop by i to ColLength, y to ColLength) or a Rectangle board (loop by i to ColLength, y to RowLength) and then the actual positions are drawn by pos[y] with the out loop drawing the separators. My C# knowledge is limited though, so I dont know the proper code (or antipatterns) involved with this. \$\endgroup\$ – Brandon Barney Aug 4 '17 at 18:15
  • \$\begingroup\$ @BrandonBarney I opted to use a single loop and skip to the position in the array based on the current row. I would rather not have nested loops when I can avoid them. This to me is fairly clear and concise. This implementation will not go out of bounds assuming the "pos" array length is a colLength * rowLength (+1 since he uses 0 in the beginning). This may be a problem, but it is not what I am trying to demonstrate in this answer. I am trying to show that he can reduce the amount of hard coded values by reusing variables and loops. \$\endgroup\$ – Yousend Aug 4 '17 at 18:34
3
\$\begingroup\$

I liked Maxim's way of checking the winning state of the game so here is my take on it (starting from OP's code, I have also added some coloring):

using System;
using System.Linq;

namespace TicTacToeConsole
{
class Program
{
    private enum Turn
    {
        Player0,
        PlayerX
    }
    private enum GameState
    {
        X,
        O,
        Tied,
        NotDone
    }

    static string[] _pos = new string[9] { "0", "1", "2", "3", "4", "5", "6", "7", "8" }; // Array that contains board positions --------------------------------
    public static string NamePlayerO { get; set; }
    public static string NamePlayerX { get; set; }

    public static int ScorePlayer1 { get; set; }
    public static int ScorePlayer2 { get; set; }

    static GameState _gameRes = GameState.NotDone;

    static void Main(string[] args) // Main ==============================================
    {
        int choice;
        var turn = Turn.Player0;

        GameIntro();

        do
        {
            while (_gameRes == GameState.NotDone) // Game loop ------------------------------------------------------
            {
                DrawBoard();
                Console.WriteLine($"{Environment.NewLine}Score: {NamePlayerO} - {ScorePlayer1}     {NamePlayerX} - {ScorePlayer2}");
                Console.WriteLine($"{(turn == Turn.Player0 ? NamePlayerO : NamePlayerX)}'s turn");

                choice = GetPlayerNextPosition();
                if (_pos[choice] == choice.ToString()) // Checks to see if spot is taken already --------------------
                {
                    _pos[choice] = turn == Turn.Player0 ? "O" : "X";
                }
                else
                {
                    Console.WriteLine("Position already taken! Try again");
                    Console.ReadLine();
                    Console.Clear();
                    continue;
                }

                turn = turn == Turn.Player0 ? Turn.PlayerX : Turn.Player0;
                Console.Clear();
                _gameRes = CheckWin();
            }

            // Game Done
            DrawBoard();

            switch (_gameRes)
            {
                case GameState.X:
                    Console.WriteLine($"Player {NamePlayerX} won!");
                    ScorePlayer2++;
                    break;
                case GameState.O:
                    Console.WriteLine($"Player {NamePlayerO} won!");
                    ScorePlayer1++;
                    break;
                case GameState.Tied:
                    Console.WriteLine("It's a draw!");
                    break;
            }
        }
        while (PleaseMakeChoice());
    }

    static int GetPlayerNextPosition()
    {
        int choice;
        while (true)
        {
            Console.WriteLine("Which position would you like to take?");
            if (int.TryParse(Console.ReadLine(), out choice))
                if (choice >= 0 && choice <= 8) break;
                else Console.WriteLine("Not a valid positon! Try again!");
        }
        return choice;
    }

    static bool PleaseMakeChoice()
    {
        bool playing = true;
        int choice;

        Console.WriteLine("Score: {0} - {1}     {2} - {3}", NamePlayerO, ScorePlayer1, NamePlayerX, ScorePlayer2);
        Console.WriteLine("");
        Console.WriteLine("What would you like to do now?");
        Console.WriteLine("1. Play again");
        Console.WriteLine("2. Leave");
        Console.WriteLine("");

        while (true)
        {
            Console.WriteLine("Enter your option: ");
            if (int.TryParse(Console.ReadLine(), out choice))
                if (choice == 1 || choice == 2) break;
        }

        switch (choice)
        {
            case 1:
                _pos = _pos.Select((x, i) => _pos[i] = i.ToString()).ToArray(); // Resets board ------------------------
                Console.Clear();
                _gameRes = GameState.NotDone;
                break;
            case 2:
                Console.Clear();
                Console.WriteLine("Thanks for playing!");
                Console.ReadLine();
                playing = false;
                break;
        }

        return playing;
    }

    static bool CanStillPlay()
    {
        return _pos.Where(x => x == "X" || x == "O").Count() < _pos.Length;
    }

    static void GameIntro()
    {
        Console.WriteLine("Hello! This is Tic Tac Toe.");
        Console.WriteLine("What is the name of player 1?");
        NamePlayerO = Console.ReadLine();
        Console.WriteLine("Very good. What is the name of player 2?");
        NamePlayerX = Console.ReadLine();
        Console.WriteLine("Okay good. {0} is O and {1} is X.", NamePlayerO, NamePlayerX);
        Console.WriteLine("{0} goes first.", NamePlayerO);
        Console.ReadLine();
        Console.Clear();
    }

    static readonly int[][] _winPos = new[]
    {
        new[] {0, 1, 2},
        new[] {3, 4, 5},
        new[] {6, 7, 8},
        new[] {0, 3, 6},
        new[] {1, 4, 7},
        new[] {2, 5, 8},
        new[] {0, 4, 8},
        new[] {2, 4, 6}
    };
    static GameState CheckWin()
    {
        var wonBy = _winPos.Select(x => x.Select(i => _pos[i]).Distinct())
                    .Where(x => x.Count() == 1).ToList();

        if (wonBy.Count() == 0)
            return CanStillPlay() ? GameState.NotDone : GameState.Tied;
        else
            return wonBy.SingleOrDefault().SingleOrDefault() == "X" ? GameState.X : GameState.O;
    }

    static void DrawBoard() // Draw board method ==========================================
    {
        DrawPosLeft(_pos[0]);
        DrawPosMiddle(_pos[1]);
        DrawPosRight(_pos[2]);
        Console.ResetColor();
        Console.WriteLine("-------------------");
        DrawPosLeft(_pos[3]);
        DrawPosMiddle(_pos[4]);
        DrawPosRight(_pos[5]);
        Console.ResetColor();
        Console.WriteLine("-------------------");
        DrawPosLeft(_pos[6]);
        DrawPosMiddle(_pos[7]);
        DrawPosRight(_pos[8]);
        Console.ResetColor();
        Console.WriteLine("-------------------");
    }
    static void DrawPosLeft(string pos)
    {
        SetConsoleColor(pos);
        Console.Write("   {0}", pos);
        Console.ResetColor();
        Console.Write("  |  ");
    }
    static void DrawPosMiddle(string pos)
    {
        SetConsoleColor(pos);
        Console.Write("{0}", pos);
        Console.ResetColor();
        Console.Write("  |  ");
    }
    static void DrawPosRight(string pos)
    {
        SetConsoleColor(pos);
        Console.WriteLine("{0}   ", pos);
        Console.ResetColor();
    }
    static void SetConsoleColor(string pos)
    {
        switch (pos)
        {
            case "O":
                Console.ForegroundColor = ConsoleColor.Red;
                break;
            case "X":
                Console.ForegroundColor = ConsoleColor.Magenta;
                break;
            default:
                Console.ForegroundColor = ConsoleColor.Green;
                break;
        }
    }
}
}

Here are some reasons I wrote it this way:

  • I like enums. When I read GameState.NotDone, I find it convenient and easy to understand
  • Notice the _pos array is now zero-based. I was not comfortable with the idea of having the first index position unused.
  • DrawBoard adds coloring. Totally unnecessary and doesn't improve anything but looks pretty.
  • The work is divided in multiple methods that are not very long. I would argue that methods that don't span several pages are easier to understand.
  • Used the ? operator to switch the turn from one player to another. It's nicer than an if else statement (once you get used to it).
  • Used a C# 7 feature called string interpolation. It makes it easier to write out strings than the old way of string.Format("{0} bla bla {1}", arg1, arg2).
  • Notice how details are left out of the main game loop. The implementation is hidden behind other methods. (e.g. GetPlayerNextPosition, CheckWin, DrawBoard). This was also done in the OP's code. Good, I just went a bit further, to make the main loop shorter, so that it becomes more readable.
  • I used int.TryParse instead of int.Parse. More robust! Should try it sometimes.
  • CheckWin is nice but difficult to understand. It is a clever idea by someone else. I just made it work.
\$\endgroup\$
  • \$\begingroup\$ I think providing another solution to the same problem gives the OP a different point of view. I learn by looking at other people's code. The OP solved the problem one way. Here's another. It's a form of review. Instead of saying "enums should be used", I show how I use them in a problem that the OP already understands and knows how to solve. I too, learned from Maxim's way of checking for the win. Thought, it would be nice to contribute. \$\endgroup\$ – tzup Aug 7 '17 at 7:59
2
\$\begingroup\$

You have some good instincts.

Code should tell you 3 things: * What it's doing. * How it's doing it. * Why it's doing it.

You address these things using different constructs in code.

  • Method name explains "what you are doing"
  • Method body explains "How you are doing that"
  • Your motivation is what comments are for.

When learning to code all code examples use comments to explain "what is happening" this is bad practice. Whenever you feel the need for a comment, that is good feedback. Write the comment, then say "is this a what? a how? or a why?"

Check your code. Most of these comments are marking out where a new method should be. :) Try it!

\$\endgroup\$
1
\$\begingroup\$

First thing that jumps out at me is that you assume that the Players enter a number. Never trust users. What seems logical to you is not always what a User will think. Int.Parse will throw an exception if the string handed to it is not actually a number. I would change the following code:

while (correctInput == false)
            {
                Console.WriteLine("Which position would you like to take?");
                choice = int.Parse(Console.ReadLine());
                if (choice > 0 && choice < 10)
                {
                    correctInput = true;
                }
                else
                {
                    continue;
                }
            }

I would change it to something like this:

while (correctInput == false)
{
    try
    {
        Console.WriteLine("Which position would you like to take?");
        choice = int.Parse(Console.ReadLine());
        if (choice > 0 && choice < 10)
        {
            correctInput = true;
        }

    }
    catch
    {
        Console.WriteLine("That is not a number"); 
    }
}

First, removing the continue. It does nothing, since the loop will just continue anyway without it. Continue should only be used at the beginning or the middle of a loop, never the end.

Next is the try catch. In the event that the int.Parse fails because the user entered an incorrect string, either maliciously or because of fat fingers, then this will prevent the program from crashing because of uncaught exceptions.

Finally, I recommend breaking down the Main function, mostly for readability and comprehension. A good benchmark is that if you have a function that is over 100 lines long, then it should probably be split up into smaller functions. The code for playing a game should not really be in the same function as the victory text. Splitting the game itself into its own function would improve the readability, especially if you wish to return and read the code at a much later date.

\$\endgroup\$
  • 6
    \$\begingroup\$ A TryParse() would be much better than a try..catch \$\endgroup\$ – Heslacher Aug 4 '17 at 10:08
  • \$\begingroup\$ @Heslacher True, but this has been marked as beginner. I personally feel that try..catch is easier to understand than the out keyword. Plus, learning about try..catch is useful for a lot more than the out keyword, since it is used more. \$\endgroup\$ – Imperial Justinian Aug 4 '17 at 10:24
  • 2
    \$\begingroup\$ True about the beginner, but for a beginner it is also good to know that the best exception is the one which never happens. \$\endgroup\$ – Heslacher Aug 4 '17 at 10:28
  • \$\begingroup\$ @Heslacher After your words a beginner can think that using of return codes is better than exceptions :) But C# not C, exceptions is a cool mechanism of errors handling and it shouldn't be ignored. In this case TryParse is obviously better but in real applications there are a lot of situations where exceptions is the best way to handle errors. I understand that you know this, I wrote this comment for beginners. \$\endgroup\$ – Maxim Aug 4 '17 at 10:52
  • 4
    \$\begingroup\$ I would rather teach a beginner int.TryParse rather than teach them to have a catch and to do nothing with the exception. \$\endgroup\$ – Rick Davin Aug 4 '17 at 12:09
1
\$\begingroup\$

Get code out of Program.cs and main

First thing I would do with your code is to not be in the practice of putting any sort of code in Program. I don't mean to have 0 code, but it should be responsible for only 1 thing: Program state. That means just enough code to start, and just enough to exit (optionally report abnormal exits). To that extent lets take the first step. Copy the entire DrawBoard method and paste it into a new class. I've chosen GameGraphics.cs.

public class GameGraphics
{
    public void DrawBoard()
    {
        Console.WriteLine("   {0}  |  {1}  |  {2}   ", pos[1], pos[2], pos[3]);
        Console.WriteLine("-------------------");
        Console.WriteLine("   {0}  |  {1}  |  {2}   ", pos[4], pos[5], pos[6]);
        Console.WriteLine("-------------------");
        Console.WriteLine("   {0}  |  {1}  |  {2}   ", pos[7], pos[8], pos[9]);
    }
}

Now in Program.cs in you'll need to make an instance of your GameGraphic to make use of it. But before we do, I want to introduce you to another type of class. A test class.

Unit tests

There are many different TestFrameworks available so you can search for them on your own (NUnit, xUnit, msTest...etc etc). Even though I'm part of the NUnit team I'll recommend sticking with what is built into Visual Studio so that you don't have to learn about NuGet and all that it brings to the table (although it is also a good easy research project). So you've created a new Test Project (Create Unit Test Project how to). Now lets write a test for GameGrahpics. But wait! It doesn't compile! And even if it did it doesn't lend it self to a easy test. Grumble.. ok not a huge problem. Really we only have 1 test that we need to write, we could write more and with any thing it takes practice, but I'm going to write my test to make GameGraphics work the way I want it to. So here it is:

[TestClass]
public class UnitTest1
{
    [TestMethod]
    public void TestMethod1()
    {
        string[] gamePosistion = { "0", "X", "O", "X", "O", "5", "O", "X", "O", "X" };
        string expectedGameboard = GetExpectedGameboard();
        GameGraphics graphics = new GameGraphics();

        string drawnGameBoard = graphics.DrawBoard(gamePosistion);

        Assert.AreEqual(expectedGameboard, drawnGameBoard);
    }

    private static string GetExpectedGameboard()
    {
        StringBuilder sb = new StringBuilder();
        sb.AppendLine(" X | O | X");
        sb.AppendLine(" O | 5 | O");
        sb.AppendLine(" X | O | X");
        string expectedGameboard = sb.ToString();
        return expectedGameboard;
    }
}

(Note that you'll want to rename that method and class, but for now the content is what is important). You'll now have 2 compile errors. First is in GameGraphics it doesn't know about a variable named pos, and in UnitTest1 it will complain that DrawBoard does not have a method that takes in a string[]. Simple fix. Add a string[] called pos in DrawBoard. Now you'll see that UnitTest1 complains that DrawBoard does not return a string. again Easy fix. Here is what I have now:

public class GameGraphics
{
    public string DrawBoard(string[] pos)
    {
        Console.WriteLine("   {0}  |  {1}  |  {2}   ", pos[1], pos[2], pos[3]);
        Console.WriteLine("-------------------");
        Console.WriteLine("   {0}  |  {1}  |  {2}   ", pos[4], pos[5], pos[6]);
        Console.WriteLine("-------------------");
        Console.WriteLine("   {0}  |  {1}  |  {2}   ", pos[7], pos[8], pos[9]);

        return "";
    }
}

I'll leave it to you to make that test pass. If you get stuck there is a big hint in the UnitTest shown above. What i love about unit tests is that they are fast and tell you very easily if all your tests pass. Example your test should say this if you run it:

Test Name:  TestMethod1
Test FullName:  StackExchange.MsTests.UnitTest1.TestMethod1
Test Source:    C:\Users\immer\Documents\Visual Studio 2017\Projects\StackExchange\StackExchange.MsTests\UnitTest1.cs : line 13
Test Outcome:   Failed
Test Duration:  0:00:00.0339269

Result StackTrace:  at StackExchange.MsTests.UnitTest1.TestMethod1() in C:\Users\immer\Documents\Visual Studio 2017\Projects\StackExchange\StackExchange.MsTests\UnitTest1.cs:line 20
Result Message: 
Assert.AreEqual failed. Expected:< X | O | X
O | 5 | O
X | O | X
>. Actual:<>.
Result StandardOutput:  
X  |  O  |  X   
-------------------
O  |  5  |  O   
-------------------
X  |  O  |  X

Notice how fast it ran? Test Duration: 0:00:00.0339269

Once you get that test to pass you'll be able to create a new instance in your main method of GameGraphics and replace all calls to DrawBoard with your newly tested method. A quick note about static: I made a new instance of GameGraphics and made the DrawBoard NON-static. I did that because when you have many static methods in a class it is a good way checking if a new class can be made. Classes can be simple (and should be). When you see that you should ask your self if it should be responsible for ________?

Good luck

\$\endgroup\$
  • 1
    \$\begingroup\$ 3 thumbs up for the title: Get code out of Program.cs and main". This is the first step on the Yellow Brick Road of Single Responsibility Principle. \$\endgroup\$ – radarbob Aug 8 '17 at 23:04

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.