Bad Practices Turn Beginners to the Dark Side: Part One: Counter

WTF? Hacking the Counter component into a robot slave of Player

The first rule of components, is that they are ... components. That is, they should be reusable sets of code that focus on a particular concern. Code that uses the component can then configure the component for it's specific purpose, leaving it unmolested and usable by other code that needs the component's capabilities.

The earlier implementation of Counter was much closer to this spirit.

In this video, the Counter component was violated to bend it into a very specific tool that is no longer reusable. Its would be better called PlayerScoreCounter.

And, this brutalization of Counter was totally unnecessary. If the code had been written differently, the original Counter code would work just fine, while remaining a clean, generic counter.

The Original Somewhat Evil Treehouse Project Code

• ZIP FILE: 4-putting-it-all-together.zip (downloaded late July 2017)
• SOURCE: 3-updating-player-counter-addplayerform-components/react-redux-course_final
• this is the path to the project within this zip file

FILE: src/components/Counter.js

import React, { PropTypes } from 'react';

const Counter = props => {
 return (
   <div className="counter" >
     <button 
      className="counter-action decrement" 
      onClick={ () => props.updatePlayerScore(props.index, -1) }>
       -
     </button>
     <div className="counter-score"> {props.score} </div>
     <button 
      className="counter-action increment" 
      onClick={ () => props.updatePlayerScore(props.index, 1) }>
       +
     </button>
   </div>
 );
}

Counter.propTypes = {
  updatePlayerScore: PropTypes.func.isRequired,
  index: PropTypes.number.isRequired,
  score: PropTypes.number.isRequired,
};

export default Counter;

NOTES:

• this is not a generic counter, but completely changed to only be useful for working with player scores
• in what ways does a generic counter need a mysterious index and an updatePlayerScore function
• notice how the mysterious index is now coupled into the onClick event handers arrow functions as the first argument given to props.updatePlayerScore(props.index, 1)
• yes, in theory other code could still reuse this Counter this by "peeking within the Counter impementation" to kludge up other client code that wants to call this Counter
• it could do this by ignoring the unneeded index prop and then feeding it a function in the updatePlayerScore prop that deliberately ignores its first argument. But that is an extremely bad practice.
• on the bright side this kind of ugly hacking is an good example of how code bases degrade into unmaintainable messes

EXCERPT: src/components/Player.js

const Player = props => {
  return (
    <div className="player">
      <div className="player-name">
        <a className="remove-player" onClick={ () => props.removePlayer(props.index) }>✖</a>
        {props.name}
      </div>
      <div className="player-score">
        <Counter 
          index={props.index} 
          updatePlayerScore={props.updatePlayerScore}
          score={props.score} />
      </div>
    </div>
  );
}

NOTES:

• this shows how Player makes use of the new highly specific, unreusable Counter

An Alternate Version of this Code that Respects Counter

NOTE: my code is adapted for the latest versions of everything, so there are some differences, like the use of 'prop-types' package to import PropTypes

FILE: src/components/Counter.js

import React from 'react';
import PropTypes from 'prop-types';

const Counter = props => {
 return (
   <div className="counter" >
     <button className="counter-action decrement"
       onClick={() => props.changeBy(-1) }>
       -
     </button>
     <div className="counter-score">  {props.count} </div>
     <button className="counter-action increment"
       onClick={() => props.changeBy(1) }>
       +
     </button>
   </div>
 );
};

Counter.propTypes = {
  count: PropTypes.number.isRequired,
  changeBy: PropTypes.func.isRequired
};

export default Counter;

NOTES:

• I changed to Counter inteface to use "changeBy" to make clear the intent of changing the count by an increment rather than setting the count in the update prop
• this Counter only knows about its total count and takes a prop to change that value by some amount

FILE: src/components/Player.js

import React from 'react';
import PropTypes from 'prop-types';
import Counter from './Counter';

const Player = props => {
  return (
    <div className="player">
      <div className="player-name">
        <a className="remove-player" onClick={ () => props.removePlayer(props.index) }>✖</a>
        {props.name}
      </div>
      <div className="player-score">
        <Counter
          changeBy={ amount => props.updatePlayerScore(props.index, amount) }
          count={props.score} />
      </div>
    </div>
  );
};

Player.propTypes = {
  index: PropTypes.number.isRequired,
  name: PropTypes.string.isRequired,
  score: PropTypes.number.isRequired,
  removePlayer: PropTypes.func.isRequired,
  updatePlayerScore: PropTypes.func.isRequired,
};

export default Player;

NOTES:

• this version of Player uses the generic counter without hacking it up
• you just need to use it properly by keeping the specifics of the player in this module and creating a correct changeBy prop for the Counter component
• if we weren't using ES6 arrow functions me might have to do a bind to create a curried function object that uses props.updatePlayerScore and props.index
• note that the function we pass to Counter just takes one argument, amount, which is how much we want to change the counter by
• when this function is called by the Counter onClick handlers when its increment or decrement buttons are clicked, it invokes props.updatePlayerScore(props.index, amount), thus keeping any grubby details of Player outside of the Counter and together in this module along with the rest of the Player code.

For reference here are the corresponding changes in the action and reducer:

• these are cosmetic because I wanted to make it clearer how we are changing the count when doing an update (i.e. adding a delta, not setting the total).

EXCERPT: src/actions/player.js

import * as ActionType from '../actiontypes/player';

// omitted addPlayer and removePlayer for brevity

// NOTE: make sure this is consistent with reducers/player.js
export const updatePlayerScore = (index, changeBy) => {
  return {
    type: ActionType.UPDATE_SCORE,
    index,
    changeBy
  };
};

NOTES:

• I simplified the action types since they belong to an owner like actions/player.js (e.g. UPDATE_SCORE instead of UPDATE_PLAYER_SCORE)
• I just used ActionType because we're already in a player.js file
• If there were any ambiguity we could import it more specifically, like PlayerActionType

EXCERPT: src/reducers/player.js

export default function PlayerReducer(state = initialState, action) {
  switch (action.type) {
    case ActionType.ADD:
/// OMITTED FOR BREVITY

    case ActionType.REMOVE:
/// OMITTED FOR BREVITY

    case ActionType.UPDATE_SCORE:
      return state.map( (player, index) => {
        if (index == action.index) {
          return {
            ...player,
            score: player.score + action.changeBy
          };
        }
        return player;
      });

      default:
        return state;
  }
}