2
\$\begingroup\$

At the moment, it is simply massive. I already reduced the size by about a hundred lines but it still seems way too big... Would appreciate some help :)

using System;

class Program
{
    static int turns = 1;
    static int player = 2;
    static char[] board = { ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ' };
    static char playerSignature = 'X';
    private static void Introduction()
    {
        Console.WriteLine("This is a simple TicTacToe game. Enter y if you have played before and n if you are new to this.");
        string input1 = " ";
        while (input1 != "y" || input1 != "n")
        {
            input1 = Console.ReadLine();
            switch (input1)
            {
                case "y":
                    Console.WriteLine("Alright, let's get started, you are X, your friend is O.");
                    DrawBoard(board);
                    break;
                case "n":
                    Console.WriteLine("TicTacToeRules:");
                    Console.WriteLine("1. The game is played on a grid that's 3 squares by 3 squares.");
                    Console.WriteLine("2. You are X, your friend is O. Players take turns putting their marks in empty squares.");
                    Console.WriteLine("3. The first player to get 3 of her marks in a row (up, down, across, or diagonally) is the winner.");
                    Console.WriteLine("4. When all 9 squares are full, the game is over.");
                    Console.WriteLine("If you have read the rules, press any key to continue.");
                    Console.ReadKey();
                    Console.Clear();
                    DrawBoard(board);
                    break;
                default:
                    Console.WriteLine("Please enter y/n.");
                    break;
            }
        }
    }
    private static void DrawBoard(char[] board)
    {

        string row = "| {0} | {1} | {2} |";
        string sep = "|___|___|___|";
        Console.WriteLine(" ___ ___ ___ ");
        Console.WriteLine(row, board[0], board[1], board[2]);
        Console.WriteLine(sep);
        Console.WriteLine(row, board[3], board[4], board[5]);
        Console.WriteLine(sep);
        Console.WriteLine(row, board[6], board[7], board[8]);
        Console.WriteLine(sep);
    }
    private static void Reset()
    {
        char[] reset = { ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ' };
        board = reset;
        DrawBoard(board);
    }
    private static void Draw()
    {
        Console.WriteLine("It's a draw!\n" +
        "Press any key to play again.");
        Console.ReadKey();
        Reset();
    }
    public static void Main()
    {
        Introduction();
        while (true)
        {
            bool isrow = false;
            bool iscol = false;
            int row = 0;
            int col = 0;
            while (!isrow)
            {
                Console.WriteLine("Choose a row (1-3): ");
                try
                {
                    row = Convert.ToInt32(Console.ReadLine());
                }
                catch
                {

                    Console.WriteLine("Please enter a number between 1 and 3.");
                }
                if (row == 1 || row == 2 || row == 3)
                {
                    isrow = true;
                }
                else
                {
                    Console.WriteLine("\nInvalid row!");
                }
            }
            while (!iscol)
            {
                Console.WriteLine("Choose a column (1-3): ");
                try
                {
                    col = Convert.ToInt32(Console.ReadLine());
                }
                catch
                {
                    Console.WriteLine("Please enter a number between 1 and 3.");
                }
                if (col == 1 || col == 2 || col == 3)
                {
                    iscol = true;
                }
                else
                {
                    Console.WriteLine("\nInvalid column!");
                }
            }
            int[] input = { row, col };
            if (player == 2)
            {
                player = 1;
                XorO(player, input);
            }
            else
            {
                player = 2;
                XorO(player, input);
            }
            DrawBoard(board);
            Win();
            turns++;
            if (turns == 10 && (board[0] == playerSignature && board[1] == playerSignature && board[2] == playerSignature && board[3] == playerSignature &&
            board[4] == playerSignature && board[5] == playerSignature && board[6] == playerSignature && board[7] == playerSignature && board[8] == playerSignature))
            {
                Draw();
            }
        }
    }
    private static void XorO(int player, int[] input)
    {
        playerSignature = (player == 1) ? 'X' : 'O';//if player ==1 dann ps = 'X', else 'O'
        int index = ((input[0] - 1) * 3) + (input[1] - 1);//3 - 1 = 2 , 2*3 = 6 + 2 = 8 für 3 3 | compact layout
        if (board[index] == ' ')
        {
            board[index] = playerSignature;
        }
        else
        {
            Console.WriteLine("That spot is already taken! Try again!");
        }
    }
    private static void Win()
    {
        if ((board[6] == playerSignature && board[4] == playerSignature && board[2] == playerSignature ||
        board[0] == playerSignature && board[4] == playerSignature && board[8] == playerSignature) ||
        (board[0] == playerSignature && board[1] == playerSignature && board[2] == playerSignature ||
        board[3] == playerSignature && board[4] == playerSignature && board[5] == playerSignature ||
        board[6] == playerSignature && board[7] == playerSignature && board[8] == playerSignature) ||
        (board[0] == playerSignature && board[3] == playerSignature && board[6] == playerSignature ||
        board[1] == playerSignature && board[4] == playerSignature && board[7] == playerSignature ||
        board[2] == playerSignature && board[5] == playerSignature && board[8] == playerSignature))
        {
            if (player == 1)
            {
                Console.WriteLine("Congratulations Player 1, you win!\n");
                Console.WriteLine("Play again? y/n.");
                Playagain();
            }
            else if (player == 2)
            {
                Console.WriteLine("Congratulations Player 2, you win!\n");
                Console.WriteLine("Play again y/n?");
                Playagain();
            }
        }
    }
    private static void Playagain()
    {
        string input2 = " ";
        input2 = Console.ReadLine();
        if (input2 == "y")
        {
            Reset();
            Console.Clear();
        }
        else if (input2 == "n")
        {
            Console.Clear();
            Console.WriteLine("Close the window to exit the game.");
            Console.ReadKey();
        }
        else
        {
            Console.WriteLine("Please enter y/n.");
        }
    }
}
\$\endgroup\$
13
  • 2
    \$\begingroup\$ There is a bug in Introduction method, the condition input1 != "y" || input1 != "n" is always true the game never start \$\endgroup\$
    – Ibram Reda
    Nov 22 at 13:08
  • \$\begingroup\$ yes, I noticed. I fixed that by adding the following at the end of the while loop: if(input1== "n" || input1 == "y") { break; } \$\endgroup\$
    – morloq
    Nov 22 at 13:12
  • 2
    \$\begingroup\$ @morloq "why does that work better?" A given value can never be equal to two different values at the same time. I'm thinking of a number. It could be equal to 1. It could be equal to 2. It could be equal to neither 1 nor 2. But it can never be equal to both 1 and 2. Let's re-read your logic: input1 != "y" || input1 != "n" Since it's impossible for input1 to be equal to both "y" and "n" at the same time, this means that at least one of these is always true; and when you use || and at least one value is true, then the result is always true. It helps to work this out on paper. \$\endgroup\$
    – Flater
    Nov 22 at 14:21
  • 1
    \$\begingroup\$ Frame challenge: the aim of improving your code is not to shorten it. Shortening might be what happens when the code gets improved, but it also might not be. Line count is not a good measure for code quality and should not be the ultimate goal of a code review. \$\endgroup\$
    – Flater
    Nov 22 at 14:24
  • 1
    \$\begingroup\$ @morloq: Your break is a band aid on a bad logical construct. It is better to fix the bad logical construct as opposed to relying on the band aid. Even if it works, it's very unreadable and needlessly complicates your code. \$\endgroup\$
    – Flater
    Nov 22 at 14:25
2
\$\begingroup\$

There are a couple of issues that we need to address before we can start looking at ways to simplify the code.

As already mentioned in the comments, you should modify the following line in Introduction to prevent an infinite loop:

while (input1 != "y" && input1 != "n")

There is another problem with XorO. This checks if the space is empty but the program does not do anything with this information and just continues. I would suggest that you modify it to the following:

private static bool XorO(int player, int[] input)
{
    playerSignature = player == 1 ? 'X' : 'O';
    int index = ((input[0] - 1) * 3) + (input[1] - 1);
    if (board[index] == ' ')
    {
        board[index] = playerSignature;
        return true;
    }
    else
    {
        Console.WriteLine("That spot is already taken! Try again!");
        return false;
    }
}

And then modify Main with GetNumber as suggested by @Ibram to the following:

public static void Main()
{
    Introduction();
    while (true)
    {
        int row = GetNumber(1, 3, "Choose a row (1-3): ");
        int col = GetNumber(1, 3, "Choose a col (1-3): ");
        int[] input = { row, col };
        if (XorO(player, input) == true)
        {
            DrawBoard(board);
            // ...
            player = player == 1 ? 2 : 1;
        }
    }
}

void Win()

Here you could have created a variable with a smaller name just to make it a bit more readable. You are also missing some parenthesis in some of the comparisons.

You also do not need separate code to write the messages for players 1 and 2:

private static void Win()
{
    char sig = playerSignature;
        // rows
    if ((board[0] == sig && board[1] == sig && board[2] == sig) ||
        (board[3] == sig && board[4] == sig && board[5] == sig) ||
        (board[6] == sig && board[7] == sig && board[8] == sig) ||
        // columns
        (board[0] == sig && board[3] == sig && board[6] == sig) ||
        (board[1] == sig && board[4] == sig && board[7] == sig) ||
        (board[2] == sig && board[5] == sig && board[8] == sig) ||
        // diagonals
        (board[0] == sig && board[4] == sig && board[8] == sig) ||
        (board[6] == sig && board[4] == sig && board[2] == sig))
    {
        Console.WriteLine("Congratulations Player {0}, you win!\n", player);
        Console.WriteLine("Play again? y/n.");
        Playagain();
    }
}

Program structure

Think of your Main function as the scaffolding of your program. I would suggest that you try to implement it around the following functions:

public static void PlayGame()
{
    InitGame();
    bool done = false;
    char player = 'X';
    while (done == false)
    {
        PrintBoard();
        GetInput(player);
        UpdateBoard();
        if (CheckWin(player))
        {
            PrintWinMessage(player);
            done = true;
        }
        else if (CheckDraw())
        {
            PrintDrawMessage(player);
            done = true;
        }
        player = TogglePlayer(player);
    }
}

public static void Main()
{
    PrintWelcomeMessage();
    if (PromptNewUser() == true)
        PrintInstructions();
    bool newgame = true;
    while (newgame)
    {
        PlayGame();
        newgame = PromptNewGame();
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ oh I think I get it now. So lets say instead I use what you proposed ( && instead of ||), that turns false as soon as the users enters y or n right? and therefore the loop ends. so at the beginning of the while loop the condition willl be true as input1 is still empty and therefore neither n or y. But after entering something it will eventually be n or y and so the condition of the while loop will turn false and the loop ends? \$\endgroup\$
    – morloq
    Nov 22 at 14:46
  • \$\begingroup\$ @morloq, That is it!. \$\endgroup\$
    – jdt
    Nov 22 at 14:47
2
\$\begingroup\$

The following piece has a redundant logic and violate the DRY principle

bool isrow = false;
bool iscol = false;
int row = 0;
int col = 0;
while (!isrow)
{
    Console.WriteLine("Choose a row (1-3): ");
    try
    {
        row = Convert.ToInt32(Console.ReadLine());
    }
    catch
    {

        Console.WriteLine("Please enter a number between 1 and 3.");
    }
    if (row == 1 || row == 2 || row == 3)
    {
        isrow = true;
    }
    else
    {
        Console.WriteLine("\nInvalid row!");
    }
}
while (!iscol)
{
    Console.WriteLine("Choose a column (1-3): ");
    try
    {
        col = Convert.ToInt32(Console.ReadLine());
    }
    catch
    {
        Console.WriteLine("Please enter a number between 1 and 3.");
    }
    if (col == 1 || col == 2 || col == 3)
    {
        iscol = true;
    }
    else
    {
        Console.WriteLine("\nInvalid column!");
    }
}

you can reduce to the following

int row = GetNumber(1,3, "Choose a row (1-3): ");
int col = GetNumber(1,3, "Choose a column (1-3): ");

by introducing the following method

static int GetNumber(int min,int max,string message)
{
    Console.WriteLine(message);
    int input = 0;
    while(!int.TryParse(Console.ReadLine(),out input))
    {
        Console.WriteLine($"Please enter a number between {min} and {max}.");
        Console.WriteLine(message);
    }

    if(input < min || input > max)
    {
        Console.WriteLine($"Invalid input, input should be between [{min},{max}]");
        return GetNumber(min, max, message);
    }
    return input;
}

you represent a player turn as an integer this lead to a lot of creepy plain number in your code, plain number are not good for reading you should avoid them as possible it will be better if you represent a player as enum

 enum Player
 {
     X_Player ,
     O_Player
 };

and define your current player of type player

Player Currentplayer = Player.X_Player;
\$\endgroup\$
6
  • 1
    \$\begingroup\$ "the board is 2D board, so to simplify the calculation you should represent it as 2D array not 1D" Strongly disagree. 2D arrays may be easier to reason about in your head, but it does not make it any easier from a coding perspective. Reducing fixed-size (!) boards to a 1D array is a very common approach in game development. If you really want an easier to read coordinate system, you can wrap your array in a neat wrapper, which will be significantly more readably than a 2D array. \$\endgroup\$
    – Flater
    Nov 22 at 15:07
  • 1
    \$\begingroup\$ There is a small typo in GetNubmer. I also do not think that recursion is the best option here. \$\endgroup\$
    – jdt
    Nov 22 at 15:08
  • \$\begingroup\$ I think there are some pros and cons to using a 1 or 2-dimensional array. I would not criticize a programmer for choosing either in this instance. \$\endgroup\$
    – jdt
    Nov 22 at 15:54
  • \$\begingroup\$ @Flater You are right, I just use it because I thought that tak tak to only played on 3*3 board, I remove it from the answer suggestion \$\endgroup\$
    – Ibram Reda
    Nov 22 at 21:55
  • 1
    \$\begingroup\$ @jdt yeas it was typo i already edit it .... Yes the recursion choice is not the best, Actually i was going to write super loop but get lazy to write it ! ... and it's okay to use recursion because how many times' user will make this mistake .. And the original point was to state Dry principle \$\endgroup\$
    – Ibram Reda
    Nov 22 at 22:07

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.