2
\$\begingroup\$

I have made Connect 4 in C#. If you have never played Connect 4, it is like Tic Tac Toe, except the pieces fall to the bottom of the grid, and the grid is usually 6x7.

This is the first big project I have made in C#, so I don't know if I wrote the program well. How can I improve my code?

using System;

namespace Connect4
{
    class Program
    {
        static void Main(string[] args)
        {
            Engine game = new Engine();

            char player = '1';
            int column;

            bool gameLoop = true;
            bool inputLoop;

            while (gameLoop) {

                System.Console.Clear();
                game.DisplayGrid();

                do {                    
                    inputLoop = true;

                    Console.Write("\nPlayer ");
                    Console.Write(player);
                    Console.Write(": ");

                    if (Int32.TryParse(Console.ReadLine(), out column)) {
                        if (1 <= column && column <= 7) {
                            if (game.DropPieceInGrid(player, column)) {
                                inputLoop = false;
                            }
                            else {
                                System.Console.Clear();
                                game.DisplayGrid();
                                Console.WriteLine("\nThat column is full.");
                            }
                        }
                        else {
                            System.Console.Clear();
                            game.DisplayGrid();
                            Console.WriteLine("\nThe integer must be between 1 and 7.");
                        }
                    }
                    else {
                        System.Console.Clear();
                        game.DisplayGrid();
                        Console.WriteLine("\nPlease enter an integer.");
                    }
                } while (inputLoop);

                if (game.FourInARow(player)) {
                    System.Console.Clear();
                    game.DisplayGrid();
                    Console.Write("\nPlayer ");
                    Console.Write(player);
                    Console.Write(" has won!\n");
                    Console.WriteLine("\nPress enter to quit.");
                    gameLoop = false;
                }
                else if (game.GridIsFull()) {
                    System.Console.Clear();
                    game.DisplayGrid();
                    Console.WriteLine("\nIt is a draw.");
                    Console.WriteLine("\nPress enter to quit.");
                    gameLoop = false;
                }
                else {
                    player = player == '1' ? '2' : '1';
                }
            }

            Console.ReadKey();
        }
    }

    class Engine
    {
        const int NUMBER_OF_ROWS = 6, NUMBER_OF_COLUMNS = 7;
        const char EMPTY = '0', PLAYER1 = '1', PLAYER2 = '2';

        private char[,] grid;

        int pieceCount;

        public Engine()
        {
            grid = new char[NUMBER_OF_ROWS, NUMBER_OF_COLUMNS];

            for (int y = 0; y < NUMBER_OF_ROWS; y++)
                for(int x = 0; x < NUMBER_OF_COLUMNS; x++)
                    grid[y, x] = EMPTY;
        }

        public void DisplayGrid()
        {
            for (int y = 0; y < NUMBER_OF_ROWS; y++) {
                for (int x = 0; x < NUMBER_OF_COLUMNS; x++) {
                    Console.Write(grid[y, x]);
                    Console.Write(' ');
                }
                Console.Write('\n');
            } 
        }

        // Returns true if the piece can be dropped in that column.
        public bool DropPieceInGrid(char player, int column)
        {
            column--;

            if (grid[0, column] != EMPTY)
                return false;

            for (int y = 0; y < NUMBER_OF_ROWS; y++) {
                if ((y == NUMBER_OF_ROWS - 1) || (grid[y + 1, column] != EMPTY)) {
                    grid[y, column] = player;
                    break;
                }
            }

            pieceCount++;
            return true;
        }

        public bool FourInARow(char player)
        {
            // Horizontal check:

            for (int y = 0; y < NUMBER_OF_ROWS; y++)
                for (int x = 0; x < 4; x++)
                    if (grid[y, x] == player && grid[y, x + 1] == player)
                        if (grid[y, x + 2] == player && grid[y, x + 3] == player)
                            return true;

            // Vertical check:

            for (int y = 0; y < 3; y++)
                for (int x = 0; x < NUMBER_OF_COLUMNS; x++)
                    if (grid[y, x] == player && grid[y + 1, x] == player)
                        if (grid[y + 2, x] == player && grid[y + 3, x] == player)
                            return true;

            // Diagonal check:

            for (int y = 0; y < 3; y++) {
                for (int x = 0; x < NUMBER_OF_COLUMNS; x++) {

                    if (grid[y, x] == player) {

                        // Diagonally left:
                        try {
                            if (grid[y + 1, x - 1] == player) {
                                if (grid[y + 2, x - 2] == player)
                                    if (grid[y + 3, x - 3] == player)
                                        return true;
                            }
                        }
                        catch (IndexOutOfRangeException) {}

                        // Diagonally right:
                        try {
                            if (grid[y + 1, x + 1] == player) {
                                if (grid[y + 2, x + 2] == player)
                                    if (grid[y + 3, x + 3] == player)
                                        return true;
                            }
                        }
                        catch (IndexOutOfRangeException) {}
                    }
                }
            }

            return false;
        }

        public bool GridIsFull()
        {
            return pieceCount >= NUMBER_OF_ROWS * NUMBER_OF_COLUMNS;
        }
    }
}
\$\endgroup\$
2
\$\begingroup\$

I am assuming that the code works as intended and hence will not remark on any logic.

General remarks

You need to split your code into either functions or classes. The for loops and Main are barely readable. This is the very first thing (right after writing up some unit tests so you know that you didn't break anything) that you should do. Without it splitted it is even hard to comment on the code because most of the comments require some sort of encapsulation of logic. Because of that, I'll just make a huge post about refactoring your code to functions, maybe someone else will benefit from that as well. I will simplify and omit some things for brevity.

Main loop

Starting from the beginning, you could possibly extract whole while loop to a function (or even a class!). To simplify things, I'll use only three classes for now, when you get it working we can go deeper. Let's start with a small dependency diagram:

graph

The above means that Program class includes Game which includes Engine, this would be parent-like dependency.

Your main method would be simply something like that:

    public static void Main(string[] args){
        var engine = new Engine();
        var game = new Game(engine);
        game.Start();
    }

Where Game.Start() would be your main loop. Note that I am injecting an engine to a game by passing engine object in constructor. This gives you an opportunity to remove a dependency (see this SO question).

Next clear place for improvement is drawing a board. According to DRY principle you should abstract the following:

System.Console.Clear();
game.DisplayGrid();
Console.WriteLine("\nPlease enter an integer.");

to a function (remember, this is happening inside game object) :

void DrawWithMessage(string message)
{
    DisplayGrid();
    Console.WriteLine(message); 
}

where DisplayGrid() looks like this:

void DisplayGrid(string message)
{
    System.Console.Clear();
    engine.DisplayGrid();
}

Why am I splitting this to two functions you might ask? Reason is SRP-ish approach, if you'd ever need to change how the grid is being displayed, it's better to change a dedicated function rather than one containing other things.

Next, checking the input from user also should be extracted to two functions.

// below is inside the input loop
var userInput = Console.ReadLine();
if(IsInputCorrect(userInput)){ 
    game.DropPieceInGrid(player, column); // this no longer returns bool
    inputLoop = false;
}

Now, your validation is encapsulated in one function. Additionally, DropPieceInGrid no longer validates the position (it is done in IsInputCorrect already) because of SRP again - funcion says that it drops piece in the grid, validation change should not apply to this function at all.

Now, the validation would look like this (notice how better it looks after the DrawWithMessage change):

bool IsInputCorrect(string userInput){
    if (Int32.TryParse(Console.ReadLine(), out column)) {
        if (1 <= column && column <= 7) {
            if (CanDropOnGrid(player, column)) { // this would be a new function with validation logic from DropPieceInGrid
                return true;
            }
            else {
                DrawWithMessage("\nThat column is full.");
            }
        }
        else {
            DrawWithMessage("\nThe integer must be between 1 and 7.") 
        }
    }
    else {
        DrawWithMessage("\nPlease enter an integer.")
    }

}

Of course, this should be further splitted into smaller functions or even moved into some Validator class (SRP again).

The above should give you overall understanding how to divide your code. This is a list of patterns that you might want to apply in your programming habbits to make the code more clear in the future: DRY, KISS and Unix rules

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy