Refactoring a Component ReactJS

Question

I am quite new in React and i'm trying to make a simple clicker game. For now everything is working well (in my opinion) but i think the code it very repetitive (I use a lot of setState). Can someone tell me how to make it more clean and deal with state? I don't ask you to refactor this code for me just for advice how to do it. Thanks for any advices.

Here's my code:

import React, { Component } from 'react';

class Enemy extends Component {
  constructor(props) {
    super(props);

    this.state = {
      hp: 10,
      nextHp: 12,
      clickDamage: 1,
      gold: 0,
      dps: 0.1,
      num: 1,
      tapCount: 0,
      itemCount: 0,
      tapCost: 10,
      itemCost: 50
    };

    this.handleClick = this.handleClick.bind(this);
    this.buyTap = this.buyTap.bind(this);
    this.buyItem = this.buyItem.bind(this);
  }

  componentDidMount() {
    const interval = setInterval(() => {
      this.setState({ hp: this.state.hp -= this.state.dps });
      if(this.state.hp <= 0) {
        this.setState({ hp: this.state.hp = this.state.nextHp });
        this.setState({ nextHp: Math.floor(this.state.nextHp * 1.2) })
        this.setState({ gold: this.state.gold + 10});
        this.setState({ num: Math.floor(Math.random() * 4 + 1)});
      }
    }, 100);
  }

  handleClick() {
    this.setState({ hp: this.state.hp - this.state.clickDamage });

    if(this.state.hp <= 0) {
      this.setState({ hp: this.state.hp = this.state.nextHp });
      this.setState({ nextHp: Math.floor(this.state.nextHp * 1.2) })
      this.setState({ gold: this.state.gold + 10});
      this.setState({ num: Math.floor(Math.random() * 4 + 1)});
    }
  }

  buyTap() {
    if (this.state.gold >= this.state.tapCost) {
      this.setState({ gold: this.state.gold -= this.state.tapCost });
      this.setState({ tapCount: this.state.tapCount += 1 });
      this.setState({ clickDamage: this.state.clickDamage += 1 })
    }
  }

  buyItem() {
    if (this.state.gold >= this.state.itemCost) {
      this.setState({ gold: this.state.gold -= this.state.itemCost });
      this.setState({ itemCount: this.state.itemCount += 1 });
      this.setState({ dps: this.state.dps += 0.1 });
    }
  }

  render() {
    return (
      <div>
        <div className="container ui center aligned">
          <h1>Enemy hp: {Math.round(this.state.hp)}</h1>
          <h3>Gold: {this.state.gold}</h3>
          <h3>Click damage: {this.state.clickDamage}</h3>
          <h3>Damage per second: {Math.round(this.state.dps * 10)}</h3>
          <img
            alt=""
            className="dragon"
            src={require("../img/dragon" + this.state.num + ".jpg")}
            onClick={this.handleClick}
            draggable={false}
          />
        </div>
        <div className="ui container">
          <img
            onClick={this.buyTap}
            alt=""
            style={{ width: '50px' }}
            src={require("../img/tap.png")}
          />
          <p>Count: {this.state.tapCount}</p>
          <p>Cost: {this.state.tapCost}</p>
          <img
            onClick={this.buyItem}
            alt=""
            style={{ width: '50px' }}
            src={require("../img/rapier.png")}
          />
          <p>Count: {this.state.itemCount}</p>
          <p>Cost: {this.state.itemCost}</p>
        </div>
      </div>
    );
  }
}

export default Enemy;

Hi! I think this question is better asked on codereview.stackexchange.com — Andrew
– Andrew, Commented Mar 15, 2020 at 2:00

Drew Reese · Accepted Answer · 2020-03-15 03:13:03Z

Not much to factor. But I can say that all your state updates can be merged. You can update multiple state values at once.

this.setState({ hp: this.state.hp = this.state.nextHp });
this.setState({ nextHp: Math.floor(this.state.nextHp * 1.2) })
this.setState({ gold: this.state.gold + 10});
this.setState({ num: Math.floor(Math.random() * 4 + 1)});

to

this.setState({
  hp: this.state.nextHp,
  nextHp: Math.floor(this.state.nextHp * 1.2),
  gold: this.state.gold + 10,
  num: Math.floor(Math.random() * 4 + 1)
});

Side Note: State mutations like this.state.hp = this.state.nextHp are a react anti-pattern and can lead to bugs.

Second Side Note: For state updates that depend on current state values it is better to use a functional state update so state updates are correctly queued up and processed in the case that more than one update is triggered in a render cycle.

this.setState(prevState => ({
  hp: prevState.nextHp,
  nextHp: Math.floor(prevState.nextHp * 1.2),
  gold: prevState.gold + 10,
  num: Math.floor(Math.random() * 4 + 1)
}));

Here's a codesandbox demo of mine that illustrates why this functional updating can be important.

kmui2 · Accepted Answer · 2020-03-15 04:59:46Z

In addition to combining the setStates into one as in @Drew Reese's answer, you can also avoid having to bind the methods (i.e. this.handleClick = this.handleClick.bind(this);) by defining them as arrow functions in the class:

  handleClick = () => {
    // ...
  }

  buyTap = () => {
    // ...
  }

  buyItem = () => {
    // ...
  }

With that, you can also remove the constructor from

  constructor(props) {
    super(props);

    this.state = {
      hp: 10,
      nextHp: 12,
      clickDamage: 1,
      gold: 0,
      dps: 0.1,
      num: 1,
      tapCount: 0,
      itemCount: 0,
      tapCost: 10,
      itemCost: 50
    };

    this.handleClick = this.handleClick.bind(this);
    this.buyTap = this.buyTap.bind(this);
    this.buyItem = this.buyItem.bind(this);
  }

to just

// (state is defined without a constructor as an instance variable)
state = {
    hp: 10,
    nextHp: 12,
    clickDamage: 1,
    gold: 0,
    dps: 0.1,
    num: 1,
    tapCount: 0,
    itemCount: 0,
    tapCost: 10,
    itemCost: 50
};

Collectives™ on Stack Overflow

Refactoring a Component ReactJS

2 Answers 2

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related