2
\$\begingroup\$

I am an experienced programmer, but a complete beginner with React. My goal is to teach myself React.

I've made a simple die-roller application. The application presents a drop-down, and the user can choose what die to roll (e.g. a d6 is a six-sided die). When the user clicks "roll", an appropriate random number is generated and displayed.

The app is working fine, but I would be grateful to know whether I've written genuinely "React-y" code; whether it's structured properly, whether I have got the naming conventions right, etc.

Some specific questions: is the "Dice" constant declared in the right place/way? Should the "option" elements be full React components? Should the "button" be a full React component?

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

const Dice = ["4", "6", "8", "10", "12", "20"];

class App extends Component {

  constructor(props) {
    super(props);
    this.state = {
      outcome: null,
      sides: null
    };
    this.handleClick = this.handleClick.bind(this);
    this.setSides = this.setSides.bind(this);
  }

  render() {
    return (
      <div className="App">
        <header className="App-header">
          Die roller
        </header>

        <DieChooser dice={Dice} setSides={this.setSides}></DieChooser>
        <button onClick={this.handleClick}>Roll</button>
        <Result outcome={this.state.outcome} />

      </div>
    );
  }

  setSides(s) {
    this.setState({sides: s});
  }

  handleClick() {
    const rolled = Math.floor(Math.random() * (this.state.sides)) + 1;
    this.setState({ outcome: rolled })
  }
}

class DieChooser extends Component {

  constructor(props) {
    super(props);
    this.handleChange = this.handleChange.bind(this);
    this.props.setSides(props.dice[0]);
  }

  handleChange(e) {
    this.props.setSides(e.target.value)
  }

  render() {
    return (
      <select onChange={this.handleChange}>
        {this.props.dice.map((die) =>
          <option key={die} value={die}>d{die}</option>
        )}
      </select>
    )
  }
}

function Result (props) {
  return (
    <p>{props.outcome}</p>
  )
}

export default App;
\$\endgroup\$
  • 1
    \$\begingroup\$ if you use arrow function like handleClick = () => {}; in the class, you don't need to bind it in the constructor. \$\endgroup\$ – Peter Dec 4 '18 at 18:52
2
\$\begingroup\$

With the new ES6 syntax, arrow functions allow you to remove .bind from your code.

Don't make a stateful component if you're not using its state. Your DieChooser doesn't handle its value change by itself since its parent component is handling it. Make a stateless(function) component whenever you can and do not require to use lifecycle methods or a private state.

I also moved the setSides up to the parent to improve readability.

A select tag will always select the first option by default; there is no need for this.props.setSides(props.dice[0]);.

Props deconstructing will also make your code more readable, as the last component Result only takes a single line now.

Working live example :

const Dice = [4, 6, 8, 10, 12, 20];

class App extends React.Component {

	constructor(props) {
		super(props);

		this.state = {
			outcome: null,
			sides: null
		};
	}

	render = () => {
		return (
			<div className="App">
				<header className="App-header">
					Die roller
                </header>
				<DieChooser dice={Dice} setSides={this.setSides} />
				<button onClick={this.handleClick}>Roll</button>
				<Result outcome={this.state.outcome} />
			</div>
		);
	}

	setSides = event => {
		this.setState({ sides: event.target.value });
	}

	handleClick = ev => {
		const rolled = Math.floor(Math.random() * (this.state.sides)) + 1;
		this.setState({ outcome: rolled })
	}
}

const DieChooser = ({ dice, setSides }) => {
	return (
		<select onChange={setSides}>
			{dice.map( die =>
				<option key={die} value={die}>d{die}</option>
			)}
		</select>
	)
}

const Result = ({ outcome }) => <p>{outcome}</p>


ReactDOM.render(<App/>, document.getElementById('root'))
<script src="https://cdnjs.cloudflare.com/ajax/libs/react/16.3.0/umd/react.production.min.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/react-dom/16.3.0/umd/react-dom.production.min.js"></script>
<div id='root'/>

\$\endgroup\$
  • \$\begingroup\$ Just few very minor things: setSides() (rightly) moved up in the tree but it still depends on an implementation detail (e.target.value). I'd rather abstract this away (if, for example, I'll change DieChooser to click an image in a grid...then I also have to change code here). App might be a PureComponent (not that it makes any real difference here, I just consider it a - opinionated - good habit for non functional ones). \$\endgroup\$ – Adriano Repetti Jan 9 at 18:01

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.