Ytdyklly

I was told to code a board game app using React for a job interview.

They told me they chose other candidates to move forward with, so I was wondering on what I could improve.

The full code can be viewed here.

Here are the more interesting components:

TicTacToeGame

import React, {useCallback} from 'react'; // useCallback imported by mistake here..

import './tic-tac-toe.scss';

import update from 'immutability-helper';

import Board from '@/board/board';

import ScoreList from '@/score-list/score-list';

import {isGameOver, isDraw} from '../../utils/tic-tac-toe';

import x from '#/images/x.svg';

import o from '#/images/o.svg';

import restart from '#/images/restart.svg';



const X = 0;

const O = 1;

const EMPTY = 2;



const xImg = <img className="xo" src={x}/>;

const oImg = <img className="xo" src={o}/>;



const IMAGE_MAP = {

    [X]: xImg,

    [O]: oImg

};



const INITIAL_STATE = {

    turn: 0,

    players: [{

        score: 0

    }, {

        score: 0

    }],

    board: [

        [EMPTY, EMPTY, EMPTY],

        [EMPTY, EMPTY, EMPTY],

        [EMPTY, EMPTY, EMPTY]

    ]

};



class TicTacToe extends React.Component {

    constructor(props) {

        super(props);



        this.state = INITIAL_STATE;

    }



    renderBox = (row, column) => {

        const {board} = this.state;

        const value = board[row][column];

        if (value === EMPTY) {

            return null;

        }

        return IMAGE_MAP[value];

    };



    boxClickHandler = (row, column) => {

        const {turn, players, board} = this.state;

        const value = board[row][column];

        if (value === EMPTY) {

            const newBoard = update(board, {[row]: {$splice: [[column, 1, turn % 2]]}});

            if (isGameOver(newBoard, row, column)) {

                // React will batch both setStates since it's in onClick event handler

                const newPlayers = update(players, {

                    [turn % 2]: {

                        score: {

                            $apply: function (s) {

                                return s + 1;

                            }

                        }

                    }

                });

                this.setState({

                    turn: INITIAL_STATE.turn,

                    players: newPlayers,

                    board: INITIAL_STATE.board

                });

            } else if (isDraw(board, turn)) {

                this.setState({

                    turn: INITIAL_STATE.turn,

                    players,

                    board: INITIAL_STATE.board

                });

            }

            else {

                this.setState({

                    turn: turn + 1,

                    board: newBoard

                });

            }

        }

    };



    resetGame = () => {

        this.setState({

            ...INITIAL_STATE

        });

    };



    render() {

        const {players, board} = this.state;

        return <div className="tic-tac-toe">

            <div className="tic-tac-toe-row">

                <div className="tic-tac-toe-wrapper">

                    <h1 className="title">

                        Tic - Tac - Toe

                    </h1>

                </div>

            </div>

            <div className="tic-tac-toe-row">

                <div className="tic-tac-toe-wrapper">

                    <Board

                        board={board}

                        width={400}

                        height={400}

                        isGameOver={isGameOver}

                        boxTemplate={this.renderBox}

                        onBoxClick={this.boxClickHandler}/>

                </div>

            </div>

            <div className="tic-tac-toe-row">

                <div className="tic-tac-toe-wrapper">

                    <div className="scores-and-reset">

                        <ScoreList

                            width={340}

                            height={100}

                            players={players}/>

                        <button className="reset-button" onClick={this.resetGame}>

                            Reset

                            <img src={restart}/>

                        </button>

                    </div>

                </div>

            </div>

        </div>;

    }

}



export default TicTacToe;

Board

import React, {useCallback} from 'react';

import './board.scss';

import Box from '@/box/box';

import classNames from 'classnames';



const Board = ({board, width, height, boxTemplate, onBoxClick}) => {

    return <div className="board" style={{width, height}}>

        {board.map((row, i) => <div key={i} className="row" style={{height: height / board.length}}>

                {row.map((column, j) => {

                    const boxClass = classNames({

                        top: i !== 0,

                        right: j !== row.length - 1,

                        bottom: i !== board.length - 1,

                        left: j !== 0

                    });

                    const memoizedBoxClickCallback = useCallback(

                        (e) => {

                            onBoxClick(i, j);

                        },

                        [i, j],

                    );

                    return <Box key={i + j} className={boxClass} width={width / row.length} height={height / board.length} onClick={memoizedBoxClickCallback}>

                        {boxTemplate(i, j)}

                    </Box>;

                })}

            </div>

        )}

    </div>;

};



export default Board;

Box

import React from 'react';

import './box.scss';



class Box extends React.PureComponent {

    render() {

        const {children, className, width, height, onClick} = this.props;

        return <div className={`box ${className} pointer`} style={{width, height}} onClick={onClick}>

            {children}

        </div>;

    }

}



export default Box;

Design choices:

• I tried to make the base components reusable for other implementation of board games so I pass onClick handler for the Box component and boxTemplate function as a children to the Box component to render whatever the game wants (e.g TicTacToeGame will render XO respectively, SnakesNLaddersGames will render snakes and ladders respectively and so on..), also the Board component sets class top right left bottom respectively (depends on where the box is relative to the edge of the board) without styles for each Box so the game component it-self could decide on the styling of these components based on the location of the Box.




• I didn't use any state management library as this is a pretty small project. I know adding such library could benefit me in the future for when the application expands, but right now I don't see a reason for the state logic update (which is inside the function boxClickHandler in TicTacToeGame) to be inside a reducer function (even if using useReducer hook) because this logic is not a reusable logic for other board games that might get implemented using the base Board and Box components.

The only thing I didn't took into account is that the functional component Board might get rendered for nothing if a game component state which doesn't require board update will change, but since I implemented only TicTacTacGame and all its state requires re-render of the Board component I didn't took this into account. If I were to solve this I would just use PureComponenet or React.memo on the component.

Overall, it looks pretty good to me, pretty much all my comments are opinionated to a lesser or (often) greater extent. I'd summarize my main feedback as "YAGNI" and "think readability, not reusability".

Less Reusability

Unless it was stipulated in the interview, I think the concern of trying to make this reusable was unnecessary and somewhat unhelpful. IMO, premature reusability is a common source of unnecessary complication (like premature optimization).

Certainly, there are some obvious cases for reusability (like UI widgets), but trying to make core "business concerns" like a game implementation reusable is, in my experience, often more trouble than it's worth. In general, YAGNI. And when you guess about how something may be reused in the future you often build the wrong abstraction.

Specifically, I think the Board and Box are complicated by trying to be more flexible than they really need to be.

More CSS

The CSS code wasn't included, but you should probably leverage CSS more.

• Setting explicit widths and heights in the view is inflexible and could probably be handled more gracefully in CSS - depending on browser support requirements, CSS Grid might be a good fit, but more 'traditional' approaches would work too. (Even if you went with fixed width/height, it'd still be better to have that in CSS than your HTML)




• Similar with left, right, top, bottom: which I believe are better handled with :first-child and :last-child selectors.

State Management

I agree with your decision to not use a state management library, as I do think that would be overkill. I do, however, find the large amount of logic in boxClickHandler unsatisfying - it's not wrong, but I don't think it's very good readability to have the entire tic-tac-toe logic in a click handler: some separation of the view and the logic would be good.

I do think useReducer would be a good fit: personally I'd probably define the reducer function in its own file - it's good for readability from a separation of concerns perspective, and if you were going to write unit tests it'd be much better for that. It's trivial to write good unit tests for a reducer function - much harder when that logic is in a click handler.

(Though even just defining a makeMove function on the TicTacToe component and calling it from the click handler would have been something of an improvement, IMO)

Nits

• Maybe split things into more components: the TicTacToe component has fairly large render function: personally, I like my top-level components to look more like:

render() {

    return (

        <>

            <Title />

            <Game /*...*/ /> 

            <Scores /*...*/ />

        </>

    )

}

(Even if those components are just defined elsewhere in the same file, I like when a component's render function makes the overall structure obvious)

• 
  useCallback is technically violating the rules of hooks: it's not at the top of the function as hooks are supposed to be: they're not supposed to be in 'loops' like .map. In practice, the current version works: since there's always the same number of boxes, the call order is consistent, but I'd advise against breaking the hook rules, anyways.

I'd probably share a single click handler among the boxes and either have the boxes pass in their (i, j) coordinate when they call the click handler, or the coordinates could be attached to the element via data-attributes and read by the click handler.

Alternatively, you can save complexity by not using useCallback or PureComponent - both Dan Abramov and sophiebits (core React developers) have said that you shouldn't use PureComputed everywhere unless you've measured performance first.