Welcome to the Treehouse Community

The Treehouse Community is a meeting place for developers, designers, and programmers of all backgrounds and skill levels to get support. Collaborate here on code errors or bugs that you need feedback on, or asking for an extra set of eyes on your latest project. Join thousands of Treehouse students and alumni in the community today. (Note: Only Treehouse students can comment or ask questions, but non-students are welcome to browse our conversations.)

Looking to learn something new?

Treehouse offers a seven day free trial for new students. Get access to thousands of hours of content and a supportive community. Start your free trial today.

JavaScript Building Applications with React and Redux Putting it all Together Updating the Player, Counter and AddPlayerForm Components

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;