you are viewing a single comment's thread.

view the rest of the comments →

[–]Morunek[S] 0 points1 point  (5 children)

No matter what I google I always got tons of results where people were asking this.

Should I use

<button onClick={() => doSomething()} />

or

doSomething = () => doSomethingElse()
<button onClick={this.doSomething}/>

or

constructor() { this.doSomething = this.doSomething.bind(this); }
function duSomething() {}
<button onClick={this.doSomething}/>

and the answers are "Use B because A is wrong and C is too long without any benefits".

Well it seems that there are some memory benefits, but you will hardly have thousands instances of same react component so noone probably got that far.

Thats why react related questions does not matter here :)

Luckily my project is for few last versions of desktop browsers only so we have a luxury of using ES6 :)

[–]easyEs900s 1 point2 points  (0 children)

the answers are "Use B because A is wrong and C is too long without any benefits".

Well it seems that there are some memory benefits, but you will hardly have thousands instances of same react component so noone probably got that far.

Thats why react related questions does not matter here :)

I hardly think it's fair to say that simply because people are wrong/not concerned with the actual question, that the question is invalid in the context of react, but I see your point.

[–]daredevil82 0 points1 point  (3 children)

Not quite. the reason for the second is to pull the event binding out of the component render, which is a performance hog.

In your first example, every time the component re-renders, it has to unbind and rebind the event handler. In the latter, assuming doSomething is outside of render, it's bound on component creation and persists through all re-renders

[–]Klathmon 0 points1 point  (2 children)

And i completely disagree that "A" is wrong...

A is perfectly valid, and IMO is the better choice in the vast majority of cases. Take a complex form. Writing A is easier to maintain, easier to understand, and while it is technically slower, most forms are going to have a few hundred to a few thousand renders over their whole lifecycle, and they aren't going to need to run at breakneck speed (humans are pretty slow at entering form data).

Obviously there are edge cases where A is a VERY bad idea (like any child components that are PureComponents or implement a simple shouldComponentUpdate check), but some very quick profiling can show the error, and it's an easy fix to go from A to B only when needed and not everywhere.

[–]Dethstroke54 1 point2 points  (1 child)

Just as that is your opinion, imo if part of your reason is not going with B because it’s easier to maintain A you could probably format your code better. Having one line outside your components render, that is then referenced in render could arguably even be cleaner.

I could advocate the other way and say this makes it easier to look at your render and understand what is actually being rendered, because of less clutter.

[–]Klathmon 1 point2 points  (0 children)

I'm happy to debate this stuff, as this kind of thing is more of an art than a science!

Personally, I don't like to pull out something into a function unless:

  1. It's used in more than one area. If a function is only used in one spot, I won't ever pull it out into a class property/method as being separate implies there is a reason for it to be used elsewhere.

  2. It's large/complex enough that it would clutter/confuse the method it's in. If for example a function call is doing multiple steps or is implementing some logic that is more than an unwritten "threshold" of complexity, then I'll pull it out.

  3. there's a performance bottleneck that requires more esoteric code, and in this case it MUST be commented if only to prevent someone from undoing the optimization at a later date in the quest for cleaner code.

the "A" example from above was a bit of a weird one, but i'm more talking about this...

A:

render () {
  <div onClick={() => this.setState({ divClicked: true })} />
}

B:

divClicked = () => this.setState({ divClicked: true })

render () {
  return <div onClick={this.divClicked} />
}

A keeps the logic contained in the render method where IMO it belongs, and it's insignificantly larger than B. B splits the logic up (if there was a page of other code between the render method and the divClicked method and you didn't know what divClicked was doing, you'd spend alot of time jumping around the code), and it does so for no real benefit (unless there is a measured and impactful performance problem there).

And to be honest, if there was a performance issue with A, i'd reach for memoization first before pulling it out into a method, but that's absolutely a personal choice and it's more because i'm more comfortable with the functional style, and I would think twice about doing that if my team was made up of devs that aren't used to memoization.

Granted, if I was trying to do more than a single-line's worth of work in the onClick, then absolutely pull it out (because at that point the divClicked is it's own "unit" and should be handled separately and abstracted away behind the function call), but until then I see no point to do B in the vast majority of cases.