Setting state dynamically with onClick function and string parameter.

(Dávid Ďurčo) #1

Hi, I would like to share my code with you and ask for your opinion, it is a good or bad practise ? What you would change ?

Here is my state:

this.state = {
  currentLocation: {
    "storeId": 1,
    "name": "Store Name",
    "clientWithAppCount": 0,
    "clientWithoutAppCount": 0

So there is location, client comes to that location and click on button to add himself to total count depending on whether he has an app or he doesnt has app.

So imagine two buttons, each of them has onClick event which trigger the handleAddClick function with required string parameter. I didnt want to use two functions like:

this.handleClientWithApp(), this. handleClientWithoutApp()

So i used only one function which will determine which counter to increment depending on parameter string.

<button onClick={() => this.handleAddClick("clientWithAppCount")}>I have app</button> <button onClick={() => this.handleAddClick("clientWithoutAppCount")}>I dont have app</button>

Now here is function to handle incrementing counter. I’m using string to dynamically change counter by key.

      currentLocation: {
        [Identifier]: this.state.currentLocation[Identifier] + 1

Is this a bad practise? I dont feel good about it but its better to have two functions for each of button like this:
this.handleClientWithApp, this. handleClientWithoutApp;

(Simon Williams) #2

This seems like unnecessary complication to me. You’ve parameterised handleAddClick to avoid duplicating 1 line (the setState), which seems rather pointless.

If it needs to be dynamic because you’ve got several different properties which are selected dynamically somehow then it might make sense, but if you’ve just got two buttons on the screen that update different properties then I would just give them each their own function.

(Troy Rhinehart) #3

I’ve done similar patterns in the past, however to ensure my props are pure and not generate at render, I put them in the constructor.

  this.handleWithAppClick = this.handleAddClick.bind(this, 'clientWithAppCount');
  this.handleWithoutAppClick = this.handleAddClick.bind(this, 'clientWithoutAppCount');

Also, I’d suggest not storing your state in a nested object, “currentLocation”. Keep your props/state as flat as possible so when needed you can easily apply PureComponent and/or a shallowEqual on your shouldComponentUpdate logic. Lastly, you can use setState without passing all the prior state.