3
\$\begingroup\$

I am working on a coding challenge for Tic Tac Toe. I am calculating the results and inserting them into the database.

However I have a long and messy if statement, which checks if the array's values match a valid win for O or X. I feel like it could be refactored down but I'm unsure on how to approach this.

    /** @var int */
protected $xWins = 0;

/** @var int */
protected $oWins = 0;

/** @var int */
protected $draws = 0;

/**
 */
public function __construct()
{
    // creates a new database on object creation
    $this->database = new Database();
}

    public function winner(string $input)
    {
        $input = str_replace("\\n", "", $input);

        $length = strlen($input);

        for ($i = 1; $i <= $length; $i++) {
            if ($i % 9 === 0) {

                $result = substr($input, $i - 9, 9);

                if ($result[0] === "X" && $result[1] === "X" && $result[2] === "X") {
                    $winner = "X";
                    $this->database->insert($winner, $result);
                    $this->xWins++;
                } else if ($result[3] === "X" && $result[4] === "X" && $result[5] === "X") {
                    $winner = "x";
                    $this->database->insert($winner, $result);
                    $this->xWins++;
                } else if ($result[6] === "X" && $result[7] === "X" && $result[8] === "X") {
                    $winner = "X";
                    $this->database->insert($winner, $result);
                    $this->xWins++;
                } else if ($result[0] === "X" && $result[3] === "X" && $result[6] === "X") {
                    $winner = "X";
                    $this->database->insert($winner, $result);
                    $this->xWins++;
                } else if ($result[1] === "X" && $result[4] === "X" && $result[7] === "X") {
                    $winner = "X";
                    $this->database->insert($winner, $result);
                    $this->xWins++;
                } else if ($result[2] === "X" && $result[5] === "X" && $result[8] === "X") {
                    $winner = "X";
                    $this->database->insert($winner, $result);
                    $this->xWins++;
                } else if ($result[0] === "X" && $result[4] === "X" && $result[8] === "X") {
                    $winner = "X";
                    $this->database->insert($winner, $result);
                    $this->xWins++;
                } else if ($result[2] === "X" && $result[4] === "X" && $result[6] === "X") {
                    $winner = "X";
                    $this->database->insert($winner, $result);
                    $this->xWins++;
                }

                // checks for O lines
                else if ($result[0] === "O" && $result[1] === "O" && $result[2] === "O") {
                    $winner = "o";
                    $this->database->insert($winner, $result);
                    $this->oWins++;;
                } else if ($result[3] === "O" && $result[4] === "O" && $result[5] === "O") {
                    $winner = "o";
                    $this->database->insert($winner, $result);
                    $this->oWins++;
                } else if ($result[6] === "O" && $result[7] === "O" && $result[8] === "O") {
                    $winner = "o";
                    $this->database->insert($winner, $result);
                    $this->oWins++;
                } else if ($result[0] === "O" && $result[3] === "O" && $result[6] === "O") {
                    $winner = "o";
                    $this->database->insert($winner, $result);
                    $this->oWins++;
                } else if ($result[1] === "O" && $result[4] === "O" && $result[7] === "O") {
                    $winner = "O";
                    $this->database->insert($winner, $result);
                    $this->oWins++;
                } else if ($result[2] === "O" && $result[5] === "O" && $result[8] === "O") {
                    $winner = "O";
                    $this->database->insert($winner, $result);
                    $this->oWins++;
                } else if ($result[0] === "O" && $result[4] === "O" && $result[8] === "O") {
                    $winner = "O";
                    $this->database->insert($winner, $result);
                    $this->oWins++;
                } else if ($result[2] === "O" && $result[4] === "O" && $result[6] === "O") {
                    $winner = "O";
                    $this->database->insert($winner, $result);
                    $this->oWins++;
                    // checks for draw
                } else {
                    $winner = "Draw";
                    $this->database->insert($winner, $result);
                    $this->draws++;
                }
            }
        }
\$\endgroup\$
4
  • 1
    \$\begingroup\$ May we see a realistic sample input (a couple of games passed to this method)? I think I can reverse engineer one from your code logic, but it would be better if I could have my assumptions confirmed. Why aren't x's and o's saved to the database consistently (regarding upper/lowercase)? \$\endgroup\$ – mickmackusa Jun 6 '20 at 22:41
  • \$\begingroup\$ Are unused cells of the gameboard filled with a visible character or are spaces used? I am assuming the game short-circuits to a winner as soon as a winner is determined. \$\endgroup\$ – mickmackusa Jun 6 '20 at 22:50
  • \$\begingroup\$ In your next questions, please post the complete code of a file. This code is missing the class declaration, which makes it harder than necessary for potential reviewers to run your code. \$\endgroup\$ – Roland Illig Jun 7 '20 at 8:31
  • 1
    \$\begingroup\$ Please don't modify your code after receiving answers. \$\endgroup\$ – Mast Jun 7 '20 at 10:28
1
\$\begingroup\$

This reminds me of a Battleship gameboard parser that I once designed. https://stackoverflow.com/a/47595575/2943403 a rare case where regex outperforms non-regex techniques.

I am going to recommend a class variable that is an associative array so that variable variables can be avoided. By filling each subarray with the respective $result strings, you compress your required data into a single structure instead of having a collection of results and a win/draw incrementer.

protected $outcomes = [
    'X' => [],
    'O' => [],
    'D' => []
];

Your str_replace(), strlen(), and subsequent modulus-based looping is definitely working too hard. It will be far simpler to break the string into substrings by using:

foreach (explode('\n', $input) as $result) {

Now that game results are properly separated, you can use a regular expression to concisely parse the string and return the outcome as a single letter.

A "branch reset" ((?|...)) is used so that the matching character is always \1 throughout each branch versus \1, \2, \3, \4. The x pattern flag allows me to use spaces and comments to separate and explain pattern logic.

public assessGames($input) {
    foreach (explode('\n', $input) as $result) {
        $outcome = preg_match(
            '~(?|
                ^(?:.{3}){0,2}(\S)\1{2}   #horizontal win
                |(\S)(?:.{2}\1){2}        #or vertical win
                |^(\S)(?:.{3}\1){2}       #or diagonalLTR win
                |^.{2}(\S)(?:.\1){2}      #or diagonalRTL win
              )~x',
            $result,
            $match
        )
        ? $match[1]  // X or O
        : 'D';
        $this->outcomes[$outcome][] = $result;
    }
}

Note, if you are parsing a high volume of games, you may like to refactor this approach to skip the explode&iterate step and make a single preg_match_all() call.

Now that your outcomes are fully evaluated, then saved to the class variable, make as few trips to the database as possible. I always discourage making looped trips to the db. I don't know what utilities you have in your database class, but using a batch insert technique will be beneficial here.

If/When you need to know how many wins/losses/draws a player has, just count the elements in the class variable.

Granted, the four-part pattern that I have designed may seem like incomprehensible magic to someone who is not familiar with the syntax, but by studying the pattern explanation at regex101.com, you can learn the full meaning. I have labeled each pattern to aid in understanding what each is doing.

Be sure to break your class functionality into separate methods so that each method has a "single responsibility".

\$\endgroup\$
4
  • \$\begingroup\$ You have "hardcoded" variable names like $xWins and $oWins. For my answer to cooperate with your variable naming convention, I would need to take my $match[1] and use dynamic variable syntax. stackoverflow.com/questions/9257505/… This (anti-pattern) is almost always a symptom of bad code design. To prevent this, I am naming three keys in an array to store the outcomes. This is a clean and logic design because the values are sensibly related. \$\endgroup\$ – mickmackusa Jun 11 '20 at 22:37
  • \$\begingroup\$ You can replace my "you" for "the OP" then. I am trying to explain. What is still foggy? I don't want to keep the OP's indidualized variables because then my preg_ result would need to be like ++${ strtolower($match[1]) . 'Wins'}; and that is a mess. I am recommending a wholly refined storage technique and a change to what exactly is being stored in the class level variable. \$\endgroup\$ – mickmackusa Jun 11 '20 at 23:28
  • \$\begingroup\$ No, I am not calling them the same thing. Variable variables are the problem. An associative array is the solution/replacement. I don't recommend the use of $xWins and $oWins -- I am recommending an associative array to eliminate those variables. protected $outcomes \$\endgroup\$ – mickmackusa Jun 11 '20 at 23:55
  • \$\begingroup\$ No bother, maybe I didn't explain myself very well. \$\endgroup\$ – mickmackusa Jun 12 '20 at 0:03
0
\$\begingroup\$

I wouldn't call myself an expert, but I finished a tic-tac-toe project today as well, I was writing Python, so my suggestions might not be idiomatic for PHP - but I think this will apply to your situation as well.

Your intuition is solid. Every time you find yourself using cut and paste, this is probably a sign you can put the code inside a function.

Idea One:

For example, create a function inside winner - this contains the valid win matching mode. this function takes a parameter "x_or_o" - which is either a string 'x' or a string 'o'.

This should be a great start because it will cut your checking code in half. Let's see if we can do better.

Idea Two:

I notice that your code doesn't care about whether a player has a winning row, or a winning column. In this case, you can assign your logical operators to a variable. e.g.

public function win_check(string $x_or_o) {
    top_row_win = $result[1] === x_or_o && $result[4] === x_or_o && $result[7] === x_or_o
    mid_row_win = $result[2] === x_or_o && $result[5] === x_or_o && $result[8] === x_or_o
    ...

    win = (top_row_win || mid_row_win || bottom_row_win || left_col_win || mid_col_win || right_col_win || diagonal_win || off_diagonal_win)

    if (win) {
    $winner = x_or_o;
    $this->database->insert($winner, $result)
    if (x_or_o == 'x'){
    xWins++   
}
    elif (x_or_o == 'o') {
    oWins++
}
}
}

//Then in your outer scope, your win_check function will be called from winner like this.

win_check(x)
win_check(o)

Idea Three:

In my project, I used a 2d array and used index slicing to do this more elegantly. I do not know what array slicing or index slicing is possible in PHP.

\$\endgroup\$

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.