Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I'm relatively new to react and attempted to create a simple Tic-Tac-Toe game. Would be great if i could get some feedback on this. Again, at this point using only React (no Redux).

main.js - main file which adds our App component to the DOM

import React from 'react';
import ReactDOM from 'react-dom';
import App from './container/App';

document.addEventListener('DOMContentLoaded', function() {
  ReactDOM.render(
    React.createElement(App),
    document.getElementById('mount')
  );
});

App.js - root component that contains the Board and Dashboard. One specific question I had here was: when I receive the reset() event from my Dashboard App, what's the best practice on how I can end up triggering the reset() of Board?

import React, { Component } from 'react';
import Board from './Board'
import Dashboard from './Dashboard'

export default class App extends Component {

    constructor(props) {
        super(props)

        this.state = {
            winner: ''
        }
    }

    reset() {
        console.log('how do i pass reset to Board...?')

    }

    hasWinner(winner) {
        this.setState({
            winner: winner
        })
    }

    render() {
        return (
            <div>
                <Board rows={3} cols={3} hasWinner={this.hasWinner.bind(this)} />
                <Dashboard winner={this.state.winner} reset={this.reset.bind(this)} />
            </div>
        )
    }

}

Board.js - Board component that contains Cell components! Please ignore the checkWinner() method for the purpose of this review.

import React, { Component } from 'react';
import Cell from './Cell'
import styles from '../css/board.css'

export default class Board extends Component {

    constructor(props) {
        super(props)

        let board = this.createBoard();

        this.state = {
            board: board,
            currentPlayer: 'X'
        }
    }

    createBoard() {
        let board = new Array(this.props.rows);
        for ( var row = 0 ; row < this.props.rows ; row++ ) {
            board[row] = new Array(this.props.cols);
            board[row].fill('');
        };  

        return board;   
    }

    checkWinner() {
        // TODO: please add correct winner algo!
        return this.state.board[0][0];
    }

    drawBoard() {
        let board = [];
        for ( let i = 0 ; i < this.props.rows ; i++ ) {
            for ( let j = 0 ; j < this.props.cols ; j++ ) {
                var id = i + '' + j;
                board.push(
                    <Cell 
                        key={id}
                        id={id}
                        play={this.play.bind(this)}
                        value={this.state.board[i][j]} />
                );
            }
        }
        return board;
    }

    reset() {
        let board = this.createBoard();
        this.setState({
            board: board,
            currentPlayer: 'X'
        })
    }

    play(ij) {
        let i = ij[0];
        let j = ij[1];
        let board = this.state.board;
        board[i][j] = this.state.currentPlayer;
        this.setState({
            board: board,
            currentPlayer: this.state.currentPlayer==='X'?'O':'X'
        })

        let winner = this.checkWinner();
        if ( winner != '' ) {
            this.props.hasWinner(winner);
            this.reset();
        }
    }

    getClassName() {
        return styles.board
    }

    render() {
        return (
            <div className={this.getClassName()}>
                {this.drawBoard()}
            </div>
        )
    }

}

Cell.js - Cell component

import React, { Component } from 'react';
import styles from '../css/cell.css'

export default class Cell extends Component {

    constructor(props) {
        super(props)
    }

    getClassName() {
        return styles.emptycell
    }

    play() {
        if ( this.props.value === '' )
            this.props.play(this.props.id)
    }

    render() {
        return (
            <div 
                onClick={this.play.bind(this)} 
                className={this.getClassName()}>
                    {this.props.value}
            </div>
        )
    }

}

Dashboard.js - Component to show winner and contains a reset button.

import React, { Component } from 'react'

export default class Dashboard extends Component {  

    constructor(props) {
        super(props)

    }

    render() {
        const winner = this.props.winner
                            ?<h1>And the winner is: {this.props.winner}</h1>
                            :<p></p>
        return (
            <div>
                {winner}                    
                <input type="button" value="reset" onClick={this.props.reset} />
            </div>
        )

    }
}
share|improve this question
    
Welcome to Code Review! Great job formatting your question - hopefully you get some helpful feedback! – avojak Jan 15 at 5:26
    
^ Thanks and looking forward to some helpful pointers. – N.M. Jan 15 at 5:47
up vote 1 down vote accepted

You have two options for handling the reset event.

  1. If the Board component absolutely must handle its own logic for setting its initial board state, you can add a ref prop to it, which allows you to hold onto a reference to that instance of the component, and call methods on that instance:

    reset() {
      this.board.reset();
    }
    ...
    render() {
      return (
        <div>
          <Board rows={3}
                 cols={3}
                 hasWinner={this.hasWinner.bind(this)}
                 ref={(board) => { this.board = board; }} />
          <Dashboard winner={this.state.winner} reset={this.reset.bind(this)} />
        </div>
      )
    }
    
  2. Your second (and preferred) option is to "lift up" the board state from the Board component. This is the approach that is recommended over using a ref in the React documentation. Essentially, you want to move your board state and the createBoard method from the Board component to the App component.


A note on naming:

Your Board component's hasWinner prop sounds like it should be receiving a boolean value, rather than a callback. A common convention in React is to prefix callback props with "on," so your prop names could become onWin, onPlay, and onReset, for example.


Finally, in your Dashboard component, instead of rendering an empty p tag when there is no winner, you can take advantage of the fact that React skips rendering values like false and null. So you can replace your ternary statement with something simpler:

const winner = this.props.winner && <h1>And the winner is: {this.props.winner}</h1>
share|improve this answer

Take a look below at the code with react no redux flow and react-native compatible:

Created the working example: https://rn-web-webpack-starter.herokuapp.com/

Source: https://github.com/agrcrobles/react-native-web-webpack-starter/blob/master/src/TicTacToe.js

'use strict';

const React = require('react');
const ReactNative = require('react-native');
const {
  AppRegistry,
  StyleSheet,
  Text,
  TouchableHighlight,
  View
} = ReactNative;

class Board {
  grid: Array<Array<number>>;
  turn: number;

  constructor() {
    const size = 3;
    const grid = Array(size);
    for (let i = 0; i < size; i++) {
      const row = Array(size);
      for (let j = 0; j < size; j++) {
        row[j] = 0;
      }
      grid[i] = row;
    }
    this.grid = grid;

    this.turn = 1;
  }

  mark(row: number, col: number, player: number): Board {
    this.grid[row][col] = player;
    return this;
  }

  hasMark(row: number, col: number): boolean {
    return this.grid[row][col] !== 0;
  }

  winner(): ?number {
    for (let i = 0; i < 3; i++) {
      if (this.grid[i][0] !== 0 && this.grid[i][0] === this.grid[i][1] &&
          this.grid[i][0] === this.grid[i][2]) {
        return this.grid[i][0];
      }
    }

    for (let i = 0; i < 3; i++) {
      if (this.grid[0][i] !== 0 && this.grid[0][i] === this.grid[1][i] &&
          this.grid[0][i] === this.grid[2][i]) {
        return this.grid[0][i];
      }
    }

    if (this.grid[0][0] !== 0 && this.grid[0][0] === this.grid[1][1] &&
        this.grid[0][0] === this.grid[2][2]) {
      return this.grid[0][0];
    }

    if (this.grid[0][2] !== 0 && this.grid[0][2] === this.grid[1][1] &&
        this.grid[0][2] === this.grid[2][0]) {
      return this.grid[0][2];
    }

    return null;
  }

  tie(): boolean {
    for (let i = 0; i < 3; i++) {
      for (let j = 0; j < 3; j++) {
        if (this.grid[i][j] === 0) {
          return false;
        }
      }
    }
    return this.winner() === null;
  }
}

const Cell = React.createClass({
  cellStyle() {
    switch (this.props.player) {
      case 1:
        return styles.cellX;
      case 2:
        return styles.cellO;
      default:
        return null;
    }
  },

  textStyle() {
    switch (this.props.player) {
      case 1:
        return styles.cellTextX;
      case 2:
        return styles.cellTextO;
      default:
        return {};
    }
  },

  textContents() {
    switch (this.props.player) {
      case 1:
        return 'X';
      case 2:
        return 'O';
      default:
        return '';
    }
  },

  render() {
    return (
      <TouchableHighlight
        onPress={this.props.onPress}
        underlayColor='transparent'
        activeOpacity={0.5}>
        <View style={[ styles.cell, this.cellStyle() ]}>
          <Text style={[ styles.cellText, this.textStyle() ]}>
            {this.textContents()}
          </Text>
        </View>
      </TouchableHighlight>
    );
  }
});

const GameEndOverlay = React.createClass({
  render() {
    const board = this.props.board;

    const tie = board.tie();
    const winner = board.winner();
    if (!winner && !tie) {
      return <View />;
    }

    let message;
    if (tie) {
      message = 'It\'s a tie!';
    } else {
      message = (winner === 1 ? 'X' : 'O') + ' wins!';
    }

    return (
      <View style={styles.overlay}>
        <Text style={styles.overlayMessage}>{message}</Text>
        <TouchableHighlight
          onPress={this.props.onRestart}
          underlayColor='transparent'
          activeOpacity={0.5}>
          <View style={styles.newGame}>
            <Text style={styles.newGameText}>New Game</Text>
          </View>
        </TouchableHighlight>
      </View>
    );
  }
});

const TicTacToeApp = React.createClass({
  getInitialState() {
    return { board: new Board(), player: 1 };
  },

  restartGame() {
    this.setState(this.getInitialState());
  },

  nextPlayer(): number {
    return this.state.player === 1 ? 2 : 1;
  },

  handleCellPress(row: number, col: number) {
    if (this.state.board.hasMark(row, col)) {
      return;
    }

    this.setState({
      board: this.state.board.mark(row, col, this.state.player),
      player: this.nextPlayer()
    });
  },

  render() {
    const rows = this.state.board.grid.map((cells, row) =>
      <View key={'row' + row} style={styles.row}>
        {cells.map((player, col) =>
          <Cell
            key={'cell' + col}
            player={player}
            onPress={this.handleCellPress.bind(this, row, col)}
          />
        )}
      </View>
    );

    return (
      <View style={styles.container}>
        <Text style={styles.title}>EXTREME T3</Text>
        <View style={styles.board}>
          {rows}
        </View>
        <GameEndOverlay
          board={this.state.board}
          onRestart={this.restartGame}
        />
      </View>
    );
  }
});

const styles = StyleSheet.create({
  container: {
    flex: 1,
    justifyContent: 'center',
    alignItems: 'center',
    backgroundColor: 'white'
  },
  title: {
    fontFamily: 'Chalkduster',
    fontSize: 39,
    marginBottom: 20
  },
  board: {
    padding: 5,
    backgroundColor: '#47525d',
    borderRadius: 10
  },
  row: {
    flexDirection: 'row'
  },

  // CELL

  cell: {
    width: 80,
    height: 80,
    borderRadius: 5,
    backgroundColor: '#7b8994',
    margin: 5,
    flex: 1,
    justifyContent: 'center',
    alignItems: 'center'
  },
  cellX: {
    backgroundColor: '#72d0eb'
  },
  cellO: {
    backgroundColor: '#7ebd26'
  },

  // CELL TEXT

  cellText: {
    borderRadius: 5,
    fontSize: 50,
    fontFamily: 'AvenirNext-Bold'
  },
  cellTextX: {
    color: '#19a9e5'
  },
  cellTextO: {
    color: '#b9dc2f'
  },

  // GAME OVER

  overlay: {
    position: 'absolute',
    top: 0,
    bottom: 0,
    left: 0,
    right: 0,
    backgroundColor: 'rgba(221, 221, 221, 0.5)',
    flex: 1,
    flexDirection: 'column',
    justifyContent: 'center',
    alignItems: 'center'
  },
  overlayMessage: {
    fontSize: 40,
    marginBottom: 20,
    marginLeft: 20,
    marginRight: 20,
    fontFamily: 'AvenirNext-DemiBold',
    textAlign: 'center'
  },
  newGame: {
    backgroundColor: '#887765',
    padding: 20,
    borderRadius: 5
  },
  newGameText: {
    color: 'white',
    fontSize: 20,
    fontFamily: 'AvenirNext-DemiBold'
  }
});

AppRegistry.registerComponent('TicTacToeApp', () => TicTacToeApp);

module.exports = TicTacToeApp;
share|improve this answer
    
This site is aimed at improving code through peer review. You appear to have presented an alternative approach, rather than reviewing the existing code. Can you expand on why you believe your approach is better, so that the OP can learn from your thought processes? – forsvarir Jan 16 at 7:20
    
Thanks for your response but i kinda agree with ^ comment above. Was expecting more of a critique on my code. – N.M. 2 days ago

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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