Neha / hangman

Hangman Game
1 stars 1 forks source link

Code Review #1

Open itaditya opened 6 years ago

itaditya commented 6 years ago

eslint has pointed some tips so I'm not covering them here

I can be wrong and I'm not very experienced too, I'll be happy to take feedback and have positive discussion. Feel free to correct me. Sorry if it feels too opinionated.

  1. Cherry pick dependency functions to reduce bundle size

Instead of

import _ from "lodash";

_.map() // stuff

use

import _map from 'lodash/map';

_map() // stuff

I have prefix lodash dependencies with _ but that's just my habit.

Instead of

var newState = _.assign({}, state);
newState.selectedPokemon = action.payload.pokemon;
newState.emoji = '';
newState.life = 6;

might be better to do this

var newState = _.assign({}, state, {
  selectedPokemon: action.payload.pokemon,
  emoji: '',
  life: 6
})

and even better

var newState = {
  ...state,
  selectedPokemon: action.payload.pokemon,
  emoji: '',
  life: 6
}

I personally like this and redux examples are also presented like this. However in complex cases the original way might turn out to be better.

  1. Use implicit returns inside react render functions

// src/Alphabats.js

var alpha = _.map(this.props.hangman.loadAlphabats, index => {
  return (
    <li key={index}>{index}</li>
  )
})

The above can be written in much more react idiomatic way -

var alpha = _.map(this.props.hangman.loadAlphabats, index => (
  <li key={index}>{index}</li>
))
  1. Don't put complicated logic in jsx tag attributes. It often takes more time to understand

Instead of

var alpha = _.map(this.props.hangman.loadAlphabats, index => {
  return (
    <li key={index} className="btn">
      <button
        disabled={
          this.props.hangman.life <= 0 ||
          this.props.hangman.optionsSelected[index]
        }
      >
        {index}
      </button>
    </li>
  );
});

do this

var alpha = _.map(this.props.hangman.loadAlphabats, index => {
  const isButtonDisabled = this.props.hangman.life <= 0 ||       this.props.hangman.optionsSelected[index]
  return (
    <li key={index} className="btn">
      <button
        disabled={isButtonDisabled}
      >
        {index}
      </button>
    </li>
  );
});
  1. Sprinkle a healthy dose of es6 love (looking at indexOf and {alphabet : alphabet})
itaditya commented 6 years ago

@Neha these are all my thoughts, please go through them, I hope you find them helpful