(編集: React以外のライブラリで当てはまるか確認できておらず、主語がデカめなのでタイトルに"Reactの"をつけました)

この記事はコンポーネント指向開発における、私が実際に産み出した/遭遇したアンチパターンをまとめたものです。

コンポーネント設計論はちらほら見かけますが、アンチパターン集はあまり見たことないなと思ったことと、実際の開発していくにあたってより具体的なアンチパターン例があった方が学びが捗るのではという思いからこの記事を書き始めました。似たようなコードを書いた時に参考になれば幸いです。また、絶対的な解があるものでもないと思っているので、より良い案などございましたらコメントいただけると嬉しいです。

なお、思いつきベースで書いているので特に網羅性はないです。

忙しい人のためのサマリ

中間のProp変化
- あるコンポーネントが他のコンポーネントで何がどう表示されるかを知っているようなロジックを持つのは避けよう
renderなんとか関数の大軍
- 責務に則ってコンポーネントを分けよう
固定文字ベタがき
- データと viewをきちんと分けよう
コンポーネント分割中毒
- 再利用する予定がない & 責務が膨らみすぎていないのにコンポーネントを分けまくるのはやめよう
propを展開し過ぎる
- 無駄にそのコンポーネントが必要としている情報だけをpropsとして渡すことにこだわるのはやめよう
はりきり過ぎたコンポーネント
- 状態がどこで持つのが適切かを見極めよう
安直な共通化
- 共通化は手当たり次第同じところをくくるのではなく、単一責任の原則に基づいて行おう

中間のProp変化

こんな感じの求人の年収を表すオブジェクトがあるとします。

interface Income {
  min: number;
  max: number;
}

このオブジェクトを用いて最終的に ¥ 3000 〜 5000万円 みたいな文字列を画面に出したいとします。そのために次のような関数をどこかに書く必要があります。

function displayIncome(income: Income) {
  return `¥ ${Math.floor(income.min / 10000)}〜${Math.floor(income.max / 10000)}万円`
}

こういった、データをそのままでなくViewのために加工して表示するのはよくある話だと思います。特にDATE型のデータの表示なんかではあるあるだと思います。

そして、この時次のような階層構造があるとして

先ほどの表示ロジックを、その文字列を表示する責務を持ったIncomeコンポーネントより手前で書いてしまったとしましょう。

SomeComponent


displayIncome() {
  const { min, max } = this.props.income
  return `¥ {Math.floor(min / 10000)}〜{Math.floor(max / 10000)}万円`
}

render() {
  return (
    <div>
      <Income income={this.displayIncome()} />
    </div>
  )
}

さて、無駄な親切心を発揮してくれたなという感じですが、これのなにが問題なのかというと

Incomeコンポーネント見てもpropに入ってきた文字列がどこからどう生まれたのか遡る必要がある
SomeComponentの責務が増えてしまっている
- Incomeが具体的にどう表示されるのかは、このコンポーネントは知らなくてもいいはず
- よってIncomeコンポーネントが他の場所で使われる時にこのロジックをムダに移植する必要がある

解決策

ではロジックをどこに書くのが収まりがいいかという話ですが、まず単純に考えられるのはIncomeコンポーネント内ですね。

class Income extends React.Component {
  displayIncome = () => {
    // さっきのロジック
  }

  render() {
    return (
      <p>{ this.displayIncome() }</p>
    )
  }
}

もう一つはIncomeオブジェクトにメソッドを生やすとかもあると思います。

class Income {
  public min: number;
  public max: number;
  
  get displayIncome(): string {
    // さっきのロジック
  }
}

あるコンポーネントが他のコンポーネントでどう表示されているかを知っていなきゃいけないような箇所でデータの変換ロジックを書くのは避けましょう。正しいロジック、正しい場所に。

renderなんとか関数の大軍

一つのコンポーネントの中で renderなんとか みたいな名前の関数を作ってrender関数内で呼ぶこと、あると思います。

renderHoge() {
  if(this.state.fuga) {
    return <HogeFuga />
  }
  return <Hoge />
} 

render() {
  return (
    <div>
      {this.renderHoge()}
    </div>
  )
}

こういった renderなんとか みたいな関数作ってしまう時点で役割を複数担ってしまっている臭いを醸し出しているのですが、ちょっとした分岐のために毎度コンポーネント切り出すのもだるいので多少なら分かります。

ただ、こんな感じで render　なんとかの大軍が現れるとツッコまざるを得なくなります。


render() {
  return (
    <div>
      {this.renderImage()}
      <div>
        {this.renderIcon()}
        <p>{this.props.name}</p>
      </div>

      {this.renderButton()}
    </div>
  );
}

とりあえず関数に切り出しとけば大元のrender関数自体は綺麗に見えるかもしれませんが、結局は臭いものにフタをしているだけです。個人的にはrenderが大量にあるコンポーネントは引数が3つも4つもある関数と同じようなものだと思っていて、問題点としては

テスタビリティが低い
- renderなんとかを作る理由としてはある程度のロジックを持ったコンポーネントの描画条件を切り出したいという背景だと思うので、storybookのようなコンポーネントガイドでの確認や単体テストがしづらい
純粋に読みづらい
- 最終的なアウトプットであるrender関数を一目見ただけでなにをやっているのかが分かるようにしたいです
- renderなんとか がたくさんあると把握するのに行ったり来たりするので疲れます。

解決策

ちゃんとコンポーネントに切り出しましょう。


const Image = ...
const Icon = ...
const Button = ...

render() {
  return (
    <div>
      <Image someprop />
      <div>
        <Icon />
        <p>{this.props.name}</p>
      </div>

      <Button text="hoge">
    </div>
  );
}

また、ちょっとした分岐ロジックならrender関数内に書いていいと思います。例えば「state.showがtrueなら<Hoge /> を描画する」という単純なものなら次の一行で書けます。

{this.state.show && <Hoge />}

JSでは && で評価してtrueになった場合最後の要素を返すようになっているので、それを利用した書き方です。初見ではキモさしか感じないと思いますが、慣れたらそうでもないです。

固定文字ベタがき

サイドナビとかで固定の文字をズラッと書くこと、あると思います。


const Items = ({ selectedType, onClick }) => (
  <ul>
    <Item　
      active={type.a === this.props.selectedType}
      onClick={() => this.props.onClick(type.a)}
    >
      hogehoge
    </Item>
    <Item　
      active={type.b === this.props.selectedType}
      onClick={() => this.props.onClick(type.b)}
    >
      fuga
    </Item>
    <Item　
      active={type.c === this.props.selectedType}
      onClick={() => this.props.onClick(type.c)}
    >
      hogefuga
    </Item>
  </ul>
)

安直に書きましたが、これの問題点としては

新しく要素を追加する時にコピペしたあとにどこを書き変えればいいのか探す手間が必要になる
ちょっとDOM構造を変えるだけでもそれぞれ個別に書き換える必要がある

解決策

もはやコンポーネント指向の基礎ですが、ちゃんと設定とViewを分けて記述しましょう。


const settings = [{
  type: type.a,
  text: 'hogehoge'
},{
  type: type.b,
  text: 'fuga'
},{
  type: type.c,
  text: 'hogefuga'
}]

<ul>
  {settings.map((item) => (
    <Item
      active={item.type === this.props.selectedType}
      onClick={() => this.props.onClick(item.type)}
    >
      {item.text}
    </Item>
  ))}
</ul>

こういった固定文字系はなぜだか中身の要素がコンポーネント化するのをサボられがちなシーンをよく見かけるのでしっかりやっていきましょう。

コンポーネント分割中毒

デザインシステムとか作ってるなら話は別なんですが、明確に再利用される可能性がないのに分割しまくるのは個人的には悪だと思っています。

例えばなんですがこんな感じのHTMLで構成されるコンポーネントがあるとして

<div>
  <img />
  <button>hoge</button>
</div>

この１箇所でしか現状使う見込みないのにButtonだけ切り出しちゃう。

const Button = ({ text }) => <button>{ text }</button>;

<div>
  <img />
  <Button text="hoge" />
</div>

これはちょっと極端な例ですが「なんでこれ切り出したのかよく分からん」というのはちょいちょい見かけます。あと若干別の話ですが、このレベルで分割してしまうのは初めてコンポーネント指向開発に取り組む人がかかるはしかみたいなものと勝手に思っているので、その場合は仕方ないかな感もあると言えばあります。ただ、実際問題としてはチームでこの「どういう時に個別のコンポーネントとして切り出すか」のルールを言語化して統一するのは難易度高いという話もあります。

個人の感覚ベースでは次のような兆候を見かけたら分割してます。

違うページで２箇所以上使われている
なんかやってること多い

関数をどう分割するかと同じ話でけっこう個人差あるとは思いますが、分割しても旨味ないものというのは割と明白だと思うので気をつけていきたいですね。

propを展開し過ぎる

こんな感じのオブジェクトがあるとします。


User {
  id: number;
  name: string;
  description: string;
  contribution_count: number;
  items_count: number;
  followees_count: number;
  followers_count: number;
  location: string;
  organization: string;
  website_url: string;
}

こんな感じのサマリを作るコンポーネントがあるとします。

そしてそのコンポーネントのインターフェイスをこんな感じで作ります。

<UserSummary
  id={user.id}
  itemsCount={user.items_count}
  contributionCount={user.contribution_count}
  followersCount={user.followers_count}
>

こちらの問題点としては

純粋に冗長
- 一々使う側のコンポーネントのインターフェイスを意識してマッピングを書くのはめんどくさいなと感じます({...props} みたいな書き方はできなくはないですが…)
UserSummaryで扱う情報に他のUserオブジェクトのデータが増えた時、無駄なインターフェイスの変更が生じる

解決策

4つくらいならギリギリ問題に感じないかもしれないですが、こちらはUserオブジェクトをそのまま渡してあげれば済む話かなと思います。

<UserSummary
  user={user}
>

これが例えば Button みたいな超汎用的な名前しといて

const Button = ({ user }) => (
  <button>{user.id}</button>
)

みたいなことしていたらツッコまざるをえないですが、今回の例でいうとUserSummaryというコンポーネントは完全にUserという存在に依存しています。なので、そういった場合は頑張って展開する必要はなくて、オブジェクトをそのまま渡してあげる方がなにかとメリットが多いかなと思います。

はりきり過ぎたコンポーネント

こんな感じでアコーディオン的なコンポーネントを作るとします。


state = {
  // どのインデックスのItemが開いているのかstateで管理
  openedIndex: []
}

toggleItem = (index) => {
  // indexのものがopenedIndexになかったらpush
  // あったら削除する処理
}

render() {
  return (
    <div>
      {this.props.items.map((item, index) => (
        <>
          <label onClick={() => this.toggleItem(index)}>タイトルだよ</label>
          <ul opened={this.state.openedIndex.includes(index)}>
            {this.props.items.map((item) => (
              <li onClick={this.props.onClick}>{item.text}</li>
            ))}
          </ul>
        </>
        )
      )}
    </div>  
  )
}

パッと見問題なさげですが、もうちょっとベターにできると思います。
まずアコーディオンのインターフェイスについて思いを馳せてみると、きっとこのアコーディオンを使う側のコンポーネントでは「どのアイテムが開いているのか」という状態はどうでもよく「どの値が選択されたか」だけ分かればいいはずです。
そして、もっと言うと「"どの"アイテムが開いているのか」という情報もどうでもいいはずです。

まどろっこしいのでコードで語ると、アイテム部分だけ切り出してしまえば書くロジックが減ります。


class AccordionItem extends React.Component {
  state = {
    opened: false
  }
  
  toggle = () => {
    this.setState(state => ({ opened: !state.opened })
  }

  render() {
    return (
      <>
        <label onClick={this.toggle}>タイトルだよ</label>
        <ul opened={this.state.opened}>
          {this.props.item.options.map((option) => (
            <li onClick={this.props.onClick}>{option.text}</li>
          ))}
        </ul>
      </>
    )
  }
}

const Accrodion = ({ onClick }) => (
  <div>
    {this.props.items.map((item, index) => (
      <AccordionItem
        item={item}
        onClick={onClick}
      >
    )}
  </div> 
)

違いが感じづらいかもしれないですが、Stateに関わる処理が簡単になりました。以前は配列に対する操作だったのが後者では単純なbooleanを裏返すだけになっています。「状態がどこに閉じているのか」を考えて適切なところに押し込めることによってロジックは幾分シンプルになります。

安直な共通化

こんな感じのメッセージを表示するためのUIと

猫のプロフィールを表示するUIがあるとします。

いろいろ共通点がありますね。

クリックするとエクスパンドして詳細なテキストを表示するようになる
開いたあとは、タイトル(またはヘッダー画像)とテキストがセットのものになる

共通点がいろいろあるので、一つのコンポーネントで書いて、差分はtypesというenum値を設けて分岐させて書くようにしました。


<Card>
  <Cover onClick={this.toggle}>
    {isMessage && <Title>{this.props.title}</Title>}
    {!isMessage && <CoverImage />}
  </Cover>
  <Content>
    <p>{this.props.text}</p>
  </Content>
  {isMessage && <Mask />}
</Card>

// ...とてもカオスなCSS

うん、まあこの時点で大分汚らしいのですが、今回の例は単純なリーダビリティ以外にもいろいろな問題をはらんでいます。似ているものはとことん共通化して、きっちり差分だけ分けて書くという方針を取ってしまうと次のような問題ができます。

なにが共通化されているのか分かりづらい
違う責務のものが一緒くたに書かれてしまうと変更のコストが上がる
- 片方にとって望ましくない変化を与えてしまうため、それを気にした実装をしていくことになる
- 責務が違うものはグロースしていくにあたってそれぞれ違うタイミングで違った変更が行われがち

解決策

まずなんですが、責務が違う場合はきっぱり分けてしまう方が大体のケースで余計な混乱を産まずに済みます。

なのでまず二つのコンポーネントに分けます。

MessageCard
CatProfileCard

ただ、共通化できるところはしていきましょう。単純な例でいうと「開く/閉じる」という状態があるのは共通しているので、その状態を扱うHOCでも作って取り付けるのはありかもしれません。

ただ、共通化していく時に気をつけたいのが「単一責任の原則に則って」抽出していくことです。

単一責任の原則とはなんぞやという話ですが

クラスを変更する理由は複数存在してはいけない

というアレです。

クラスというか今回の場合はコンポーネントないし振る舞いのロジックが対象ですが意味するところは同じです。

振る舞いの共通化は割と責務がふわふわしたりすることなくされるのですが、HTMLやCSSの見た目の部分の共通化は同じところが手当たり次第に共通化されてしまいがちな印象を受けます。コンポーネントという括りに対しても単一責任の原則は重要なプラクティスだと個人的には思うので、その構成要素たるHTMLやCSSでも適用していきたいです。それら単体での共通化は意味がないどころか有害だとすら考えています。

なるべく重複をしたコードを取り除こうという心意気は大切なのですが、あくまで単一責任の原則に則って抽出していきましょう。