Bad Practices Turn Beginners to the Dark Side: Part Two: AddPlayerForm.js:onSubmit

This one is more debatable than Part One.

It's not that this particular module is difficult to understand, but that making this KIND of refactoring becomes more risky with larger modules and systems (i.e. deliberately introducing clashing names that don't really add value)

In this video the onSubmit class field (with an arrow function value) was renamed to addPlayer. This was a stylistic change, not a required one.

I believe this kind of refactoring actually reduces clarity and maintainability.

This onSubmit method actually is concerned with the logic of form submission

• hence the inclusion of the command: if (e) e.preventDefault();
• this method is called BECAUSE the form had an onSubmit event!

Within the context of this module, onSubmit is a great name because it corresponds directly to the event generated by our form.

If we had more forms and more onSubmit events, we could create multiple methods distinguished like onSubmitAddPlayer, etc

Since we're in a module and we only have one form, there is no need to complicate our life and name things as if we had many forms sharing the same namespace.

Renaming this method to clash with the name of the prop actually makes the module more confusing. Now we're using the same name for two things that play different roles and the developer must give more attention to which one is meant in the code.

In searching through source code using Find String we will get more matches that the developer must now distinguish. As we write longer modules, this becomes more of a risk.

this.addPlayer or props.addPlayer? What are the odds these kinds of clashes end up becoming a future bug in a year because someone put the wrong one in the wrong place, after the code gets longer and more cluttered.

FILE: Refactored Treehouse Version with more clashing names

import React, { Component, PropTypes } from 'react';

export default class AddPlayerForm extends Component {
  static propTypes: {
    addPlayer: PropTypes.func.isRequired,
  };

  state = {
    name: ''
  };

  onNameChange = (e) => {
    const name = e.target.value;
    this.setState({ name: name });
  };

  addPlayer = (e) => {
    if (e) e.preventDefault();
    this.props.addPlayer(this.state.name);
    this.setState({ name: '' });
  };

  render() {
    return (
      <div className="add-player-form">
        <form onSubmit={this.addPlayer}>
          <input
            type="text"
            value={this.state.name}
            onChange={this.onNameChange}
            placeholder="Player Name"
          />
          <input type="submit" value="Add Player" />
        </form>
      </div>
    );
  }  
}

FILE: version that keeps onSubmit distinct

import React, { Component } from 'react';
import PropTypes from 'prop-types';

class AddPlayerForm  extends Component {

  static propTypes: {
    addPlayer: PropTypes.func.isRequired
  };

  // NOTE: this component manages its own state locally
  state = {
    name: ''
  };

  onNameChange = e => {
    const name = e.target.value;
    this.setState({ name: name });
  };

  onSubmit = e => {
    if (e) e.preventDefault();
    this.props.addPlayer(this.state.name);
    this.setState({ name: '' });
  };

  render () {
    return (
      <div className="add-player-form">
        <form onSubmit={this.onSubmit}>
          <input
            type="text"
            value={this.state.name}
            onChange={this.onNameChange}
            placeholder="Player Name"
          />
          <input type="submit" value="Add Player" />
        </form>
      </div>
    );
  }
}

export default AddPlayerForm;