7
\$\begingroup\$

Hi everyone !

I am new to react and more of a backend developper (working with Django, Symfony, Code Igniter mostly).

But I want to get better at frontend and React seems to have a great community so I thought it was a good place to start :) (not litteral start, I got basics in HTML, CSS and JS !)

So I took the official tutorial and I have been struggling a little for the last part. It is adding functionnalities to the code we wrote before :

Display the location for each move in the format (col, row) in the move history list.

Bold the currently selected item in the move list.

Rewrite Board to use two loops to make the squares instead of hardcoding them.

Add a toggle button that lets you sort the moves in either ascending or descending order.

When someone wins, highlight the three squares that caused the win.

When no one wins, display a message about the result being a draw.

My repo is here so you can test locally if you want too and I put it in a sandbox if you feel to give it a try.

I paste the JS part here since it is the interesting one :

import React from 'react';
import ReactDOM from 'react-dom';
import './index.css';

function Square(props) {
  return (
    <button
      className={props.className}
      onClick={props.onClick}
    >
      {props.value}
    </button>
  );
}

class Board extends React.Component {

  inWinnerLine(i)  {
    return this.props.winningLine ? this.props.winningLine.includes(i) : false;
  }

  renderSquare(i) {
    const won = this.inWinnerLine(i) ? 'won' : 'no-luck';
    return (
      <Square
        value={this.props.squares[i]}
        onClick={() => this.props.onClick(i)}
        className={`square ${won}`}
      />
    );
  }

  render() {
    // Need a three element array to loop
    const looper = [0, 1, 2];
    // And a counter to go to 8
    let inc = 0;

    return (
      <div>
        {looper.map((value, index) => {
          return (
            <div className="board-row">
              {looper.map((val, id) => {
                inc++;
                return this.renderSquare(inc - 1);
              })}
            </div>
          );
        })}
      </div>
    );
  }
}

class Game extends React.Component {
  constructor(props) {
    super(props);
    this.state = {
      history: [{
        squares: Array(9).fill(null),
      }, ],
      stepNumber: 0,
      xIsNext: true,
      winningLine: null,
      winner: null,
    };
  }

  calculateWinner(squares) {
    const lines = [
      [0, 1, 2],
      [3, 4, 5],
      [6, 7, 8],
      [0, 3, 6],
      [1, 4, 7],
      [2, 5, 8],
      [0, 4, 8],
      [2, 4, 6],
    ];
    for (let i = 0; i < lines.length; i++) {
      const [a, b, c] = lines[i];
      if (squares[a] && squares[a] === squares[b] && squares[a] === squares[c]) {
        this.setState({
          winner: squares[a],
          winningLine: lines[i],
        });
        return squares[a];
      }
    }

    this.setState({
      winner: null,
      winningLine: null,
    });
    return null;
  }

  handleClick(i) {
    // TODO if I go back to winner step the game cannot continue
    const history = this.state.history.slice(0, this.state.stepNumber + 1);
    const current = history[history.length - 1];
    const squares = current.squares.slice();

    if (squares[i] || this.state.winner) {
      return;
    }

    squares[i] = this.state.xIsNext ? 'X' : 'O';

    this.setState({
      history: history.concat([{
        squares: squares,
        position: getPosition(i),
      }, ]),
      xIsNext: !this.state.xIsNext,
      stepNumber: history.length,
    });

    if (this.calculateWinner(squares)) {
      return;
    }
  }

  jumpTo(step) {
    const history = this.state.history.slice(0, step);
    const current = history[history.length - 1];
    const squares = current.squares.slice();
    this.calculateWinner(squares);
    this.setState({
      stepNumber: step,
      xIsNext: (step % 2) === 0,
    });
  }

  highlight(position) {
    return position === this.state.stepNumber ? 'highlight' : '';
  }

  sort() {
    // Add action on sort button click

    this.setState({
      history: this.state.history.slice(0).reverse(),
      reverse: !this.state.reverse,
    });
  }

  render() {
    const history = this.state.history;
    const current = history[this.state.stepNumber];
    const winner = this.state.winner;

    const moves = history.map((step, move) => {
      let move_for_display = this.state.reverse ?
        history.length - 1 - move :
        move;
      const desc = step.position ?
        'Back to round n°' + move_for_display + ' ' + step.position :
        'Back to beginning';
      return (
        <li key={move}>
          <button className={this.highlight(move)} onClick={() => this.jumpTo(move)}>{desc}</button>
        </li>
      );
    });

    let status;
    if (winner) {
      status = winner + ' won.';
    } else {
      status = 'Next player: ' + (this.state.xIsNext ? 'X' : 'O');
    }

    return (
      <div className="game">
        <div className="game-board">
          <Board
            winningLine={this.state.winningLine}
            squares={current.squares}
            onClick={(i) => this.handleClick(i)}
          />
        </div>
        <div className="game-info">
          <div>{status} - <button className='sort' onClick={() => this.sort()}>Sort</button></div>
          <ol>{moves}</ol>
        </div>
      </div>
    );
  }
}

// ========================================

ReactDOM.render(
  <Game />,
  document.getElementById('root')
);

function getPosition(i) {
  let pos;
  switch (i) {
    case 0:
      pos = '(1, 1)';
      break;
    case 1:
      pos = '(2, 1)';
      break;
    case 2:
      pos = '(3, 1)';
      break;
    case 3:
      pos = '(2, 1)';
      break;
    case 4:
      pos = '(2, 2)';
      break;
    case 5:
      pos = '(2, 3)';
      break;
    case 6:
      pos = '(3, 1)';
      break;
    case 7:
      pos = '(3, 2)';
      break;
    case 8:
      pos = '(3, 3)';
      break;
    default:
      pos = 'Something went wrong';
  }
  return pos;
}

I appreciate the time you took to read this and any feedback will be welcome, I have seen similar question but they seemed old so I did a new one, sorry if I should not have.

Also my english might be bad, sorry about that too - french here ;)

\$\endgroup\$
4
  • 1
    \$\begingroup\$ So what is the problem you are having? What do you expect compared to what your are getting. A codesandbox would make it easier to help you. \$\endgroup\$ Nov 25 '20 at 15:06
  • \$\begingroup\$ Hi, thank you for your answer. Actually I get pretty much what I was expecting, the code works but as a React beginner I am looking for feedback on the way I did it and what could/should be improve for a real world app. I just put it in a sandbox -> ft6g4.csb.app \$\endgroup\$ Nov 25 '20 at 20:04
  • 1
    \$\begingroup\$ First glance looks fine. GetPos could be simplified to a dict instead of a function, a few ifs could use the ternary operator. Are you attached to a class based approach? I could add a refactored functional version if you're interested \$\endgroup\$
    – Coupcoup
    Nov 25 '20 at 20:40
  • \$\begingroup\$ I am not attached to a class based approch, I would be very interested in a function version :) this kind of dict ? \$\endgroup\$ Nov 26 '20 at 8:50
4
\$\begingroup\$

First, congratulations on working code.

Recommendations:

  • Simplify your state
  • Simplify your controls
  • Simplify your functions

Your state is overly complex, even with no user interaction it is very complex. Every action drastically changes every node in your state. I've updated your code sandbox to show these state changes.

You are actually building a data model, not state. You are then tightly coupling your data model to your controls.
As more features are added, updating the model becomes very difficult. Simple changes require you to touch too many things (controls, actions, state)

Thinking in react states "Step 3: Identify The Minimal (but complete) Representation Of UI State"

I identify your minimal (but complete) state as

  1. Moves (holds the order and location of players clicks)
  2. Last history button clicked.
  3. Is history descending.

For our first iteration, we will simply focus on 1. Moves

We can accomplish 80% of the application with only this state.
Also taking an iterative approach will show how the application can handle change.

Moves will be a simple array:

+---------------------+--------+---------------+
| Action              | square | state.moves   |
+---------------------+--------+---------------+
| No clicks           |        |[]             |
| X clicks top Left   | 0      |[0]            |
| Y clicks center     | 4      |[0, 4]         |
| X clicks top center | 1      |[0, 4, 1].     |
+---------------------+--------+---------------+

Updating our state is trivial.


        const addMove = (boardIndex) => (oldState) => {
          const { moves } = oldState;
          return {
            moves: [...moves, boardIndex]
          };
        };
    
        handleClickSquare = (boardIndex) => {
          this.setState(addMove(boardIndex));
        };
        

Ok, trivial if you understand: useState callback, spread operator, and currying

Although our state is simple, it holds a lot of implied information. From our simple state we can derive most of the data model.

We can determine:

  1. Who (X or O) owns each square.
  2. Status:
    • Is there a winner?
    • Who won?
    • Is there a tie?
    • Who is next.
  3. What squares caused the win.
  4. History of moves.

Let's examine each one individually.

// selectSquareOwners 
// Builds a tic-tac-toe board of 9 squares and who (X or O) occupies 
// each square.
// For each square, find if it is occupied and by who.  
// If the square index is in move list, it is occupied, if the move 
// index is odd it is owned by X, if it is even, it is owned by Y
export const selectSquareOwners = (state) => {
  const emptySquares = Array(9).fill("");
  return emptySquares.map((cur, i) => {
    const { moves = [] } = state || {};
    const move = moves.indexOf(i);
    const notFound = !~move;
    const isEven = move % 2;
    if (notFound) {
      return "";
    }
    return isEven ? "O" : "X";
  });
}; //  ["", "X", "", "O", "X" , ...]

// Builds a tic-tac-toe board of 9 squares.   Each square is represented by an 
// object containing: {
//   isWinner - does the square participate in the win
//   owner: who owns the square (X or O)
//   index: the id of the square 
// }
export const selectSquares = (state) => {
  const xoSquares = selectSquareOwners(state);  //  => ["X", "X", "X", "", "O", "O" , ...]
  const winningRow = getWinningRow(xoSquares);  //  => [0, 1, 2]
  return xoSquares.map((owner, index) => {
    const isWinner = !!~winningRow.indexOf(index);
    return { isWinner, owner, index };
  });
}; 

Here is the code without time travel.

You may be concerned about the efficiency of this code.
There is definitely room for improvement, but with such a small data set, the end user will not notice them so I choose simple code over efficient code here. If efficiency becomes a priority, consider using reselect.

I understand that changing all nodes when user make an input is not a good practice.

Unfortunately it is not that easy. It's all about tradeoffs and understanding secondary effects. In the tutorial before time travel, updating all nodes is still simple. As more features are added, simplifying the state should be considered. I choose small, composable functions and controls. This is easy to test and easy to reuse, but comprehending the final result may be more difficult like seeing the forest for the trees. Understanding tradeoffs will come with experience. Keep calm and Code on.

Here is the complete code

I will address control improvements soon.

Here are past code reviews for simplifying controls.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Wow, thank you for the extensive answer. There's a lot to study and I understand that changing all nodes when user make an input is not a good practice. The working code example are a great help ! I'm going to read again Thinking in React. \$\endgroup\$ Dec 2 '20 at 7:31
  • \$\begingroup\$ I've added my response above. Reach out if I can clarify anything else. \$\endgroup\$ Dec 5 '20 at 14:38

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.