Dice-rolling tool

Question

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;

• Peter: if you use arrow function like handleClick = () => {}; in the class, you don't need to bind it in the constructor.

Answer 1

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'/>

• Adriano Repetti: 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).