Jonathan Cutrell

Posted on May 24, 2019

React Anti-pattern: renderThing

#react #refactoring

If you've done much with React, you've probably run into this kind of scenario:

class Tabs extends React.Component {

  constructor(props){
    super(props)
    this.state = {}
  }

  setActiveTab(activeTab){
    this.setState({ activeTab });
  }

  renderTabs(){
    return (
      this.props.tabs.map(tab =>(
        <a onClick={e => this.setActiveTab(tab.id)}
           key={tab.id}
           className={this.state.activeTab == tab.id ? "active" : ""}
        >
          {tab.title}
        </a>
      ))
    )
  }

  render(){
    return (
      <div>
        <p>Choose an item</p>
        <p>Current id: {this.state.activeTab}</p>
        <nav>
          {this.renderTabs()}
        </nav>
      </div>
    )
  }
}

This would be used like this:

<Tabs tabs={[{title: "Tab One", id: "tab-one"}, {title: "Tab Two", id: "tab-two"}]} />

And this works! If this is all you need for this component forever, by all means, stop here!

But if this code will change in the future, you're likely to end up with a confusing and long component.

The first and most obvious refactor smell here is the renderTabs method. A few things are wrong with this.

First, the Tabs component already has a render method. So what's the difference between the Tabs render and the renderTabs methods? In one, we are rendering the list of tabs. In the other, we are adding some context. We see this often with things like filtered lists.

It may be tempting to wrap up this kind of rendering functionality inside the component specifically because the tabs need to share state somehow with the containing context.

Let's think about how we might refactor this to be easier to understand.

P.S. Let's assume you've got some kind of testing strategy. In this case, we're not going to write tests, but if you did, you'd probably want to assert that your list is rendering, and that clicking on your tabs renders out what you want it to render.

Let's start by removing the renderTabs method. This is going to look ugly at first.

class Tabs extends React.Component {

  constructor(props){
    super(props)
    this.state = {}
  }

  setActiveTab(activeTab){
    this.setState({ activeTab });
  }

  render(){
    return (
      <div>
        <p>Choose an item</p>
        <p>Current id: {this.state.activeTab}</p>
        <nav>
          {this.props.tabs.map(tab =>(
            <a onClick={e => this.setActiveTab(tab.id)}
               key={tab.id}
               className={this.state.activeTab == tab.id ? "active" : ""}
            >
              {tab.title}
            </a>
          ))}
        </nav>
      </div>
    )
  }
}

This is actually a perfectly fine component on its own. But in the future, you probably will have other places you want to use the same tab-style button, so let's see if we can make that button shareable.

Let's look at a single tab on its own.

<a onClick={e => this.setActiveTab(tab.id)}
   key={tab.id}
   className={this.state.activeTab == tab.id ? "active" : ""}
>
  {tab.title}
</a>

Now let's make this component a standalone functional component. (In other words, we want the component to take props, but we don't need it to have its own state.)

const TabButton = ({ onClick, active, title, tabId, ...props}) => (
  <a onClick={e => {e.preventDefault(); props.onClick(tabId)}}
    {...props}
    className={active ? "active" : ""}
  >
    {title}
  </a>
)

Now that we have a functional component, we can integrate this back into our original Tabs component.

const TabButton = ({ onClick, active, title, tabId, ...props}) => (
  <a onClick={e => {e.preventDefault(); props.onClick(tabId)}}
    {...props}
    className={active ? "active" : ""}
  >
    {title}
  </a>
)

class Tabs extends React.Component {
  constructor(props){
    super(props)
    this.state = {}
  }

  setActiveTab(activeTab){
    this.setState({ activeTab });
  }

  render(){
    const { tabs } = this.props;
    const { activeTab } = this.state;
    return (
      <div>
        <p>Choose an item</p>
        <p>Current id: {this.state.activeTab}</p>
        <nav>
          {this.props.tabs.map(tab =>(
            <TabButton onClick={this.setActiveTab}
               active={activeTab == tab.id}
               tabId={tab.id}
               key={tab.id}
               title={tab.title}
            />
          ))}
        </nav>
      </div>
    )
  }
}

So what do we really gain here?

Removed unnecessary/confusing renderTabs button
Created a reusable TabButton component that doesn't rely on any external state
No API changes for the Tabs interface
Easier to reason about and separate concerns - two smaller components over one larger component.

This example is contrived and small, but you are almost certainly going to find places where renderThing monsters show up.

The refactoring pattern looks like this:

Remove the monster renderThing method by moving that code back into the original render. Stop there if the code is reasonable.
Isolate a subset of the rendered output to create a new component from. (Note that you may be able to move directly to this step and jump over step 1, but I like to move it back into the render method first to see if it makes sense to just leave it there.)
Work to separate what pieces of state can go away. Ideally, shoot for a functional component; however, beware of a vanity functional component, where you keep state that should be in the subcomponent in it's parent so you can make it "functional." This is far worse than having two well-designed stateful components.
Incorporate your new component into your previous component, replacing markup. If you find yourself passing too many things directly into the child component, it's possible that you should have stopped at step #1 and not abstracted the component out at all.

Knowing when to abstract a component or routine out into its own dedicated component can be hard. Sometimes, it's purely preference; there is no one right way. When in doubt, smaller components are easier to reason about, but abstraction should have a purpose.

What other refactoring patterns are you interested in seeing a writeup on? Comment and let me know!

Top comments (13)

Sergey S. Volkov • May 24 '19

Agreed with you Jonathan!
Also I want to tell a couple words about coming advantage of separating component on sub components instead of using the one big react component
It gives advantage because rendering of the react component works by comparing previous props and next props inside react engine. And in this way React can easy check what needs to update or not

Sung M. Kim • May 25 '19

I also wanted to mention the optimization aspect of this article's refactor as well as Sergey did.

Once TabButton is wrapped in React.memo or turned into a Pure Component (as a Class Component), then it'd prevent TabButton from rendering unncessarily.

Joao L. • May 27 '19

I don't totally agree.

The author here did more than just remove the "renderThing" method. He created a new component called TabButton. That is great - improves readability, testability, and probably performance. But is it really any difference to have a function call vs an inline piece of javscript inside the curly brakets? It's still just a function. React doesn't care where you put your map function in order to check for updates.

Sergey S. Volkov • May 27 '19

I think yes, we have differences in performance between {someFunc(data)} and <someComp></someComp>
Yeah, you're right about that someComp it is the same function React.createElement but I talked about that inside this function React have ability to compare outer props with current props and after fully compared it will call re-render.
But if we will talk about {someFunc(data)} that will be a bit different between using component because it will try to call check for updates all child components.

Hermes L Herrera • May 25 '19 • Edited

I suggest to develop a component in another fashion.
Instead of array of props, is more functional and reusable, to use an array of children. Component composition.

Ex.

....
....

Render props, is a great powerfull pattern to use in this case.

Mauro Gabriel Titimoli • Nov 3 '21 • Edited

There are times where creating a new component doesn't make sense and breaking the jsx into small render functions is not a bad idea as it follows what we do in other contexts, like for example when there is a big function and we break it into smaller ones to improve legibility.

Having said this, there are times this is overused, and for these cases I agree it would be better to extract that into another component.

To summarize, it's never a good idea to go with an "all or nothing" approach, imo it's better to be on a "it's depends" motto and evaluate the case.

Having render helpers is not a bad idea or antipattern but we should not overuse this and create another component(s) given the case.

Blair McKee • May 24 '19

Great real world example! Just ran into a really similar issue earlier today and would not have thought to pull it out like that

Victor Vincent • May 25 '19 • Edited

Not using classes (but functional components and hooks) will automatically save you from such solutions.

Sung M. Kim • May 25 '19 • Edited

Would you share more how "function components + hooks" can save one from having renderThings?

Victor Vincent • May 27 '19

What you did there is basically extracting the return value of your renderThings function. The only way with FC to couple that value inside the component is to store it in a local variable which feels illegal enough for me to not to do it. This would also enforce a pattern like const renderThing = () => {} where I would immediately see that it is the component itself and I just have to rename it to Thing.