React Anti-pattern: renderThing

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:

1. Remove the monster renderThing method by moving that code back into the original render. Stop there if the code is reasonable.
2. 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.)
3. 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.
4. 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!