以前、React教育コースを受講された企業で書かれたReactのコードをリファクタリングするお仕事を行いました。
作業してみると驚くようなコードがありました！ 具体例は書けませんが、一般化して書きます。
Refactoring (Addison-Wesley Signature Series)
重複コード
このReactアプリはTypeScriptで書かれ、フィーチャー(feature)毎にディレクトリ分けされています。またアプリはstate管理にRedux Toolkitを使っています。
以下は説明用に使った架空のディレクトリ構造です。
- componentsディレクトリの下に表示用のReactコンポーネントがあります
- storeディレクトリーの下にReduxのSlice(Action + Reducer)コードがあります
- 上コードの中で使われる型定義がtypes.tsに書かれています
├── create
│ ├── types.ts
│ ├── components
│ │ └── Create.tsx
│ └── store
│ └── index.ts
├── edit
│ ├── components
│ │ └── Edit.tsx
│ └── store
│ └── index.ts
├── search
│ ├── types.ts
│ ├── components
│ │ └── Search.tsx
│ └── store
│ └── index.tsx
└── approve
├── components
│ └── Approve.tsx
└── store
└── index.tsまとも見えますが、実はtypes.tsに大きな問題がありました。
- 重要な型が直接、Reactコンポーネントや
store/index.tsファイルに書かれているものがありまいした 😅 - また複数の
types.tsファイルに同じ型定義が書かれていました 😅 - さらに複数の
types.tsファイルに書かれている同一型名の定義が一致していませんでした 😅
なぜ、このようになってしまったのでしょうか？
フィーチャー毎にディレクトリを分けるのは良いと思いますが、たぶん最初に作ったフィーチャー・ディレクトリをコピーして新たなフィーチャーを使ったのではないでしょうか。そこで型定義の不足に気が付き、そのフィーチャーのtypes.ts修正したのですが、コピー元のtypes.tsは修正し忘れたのだと思います。
store/index.tsもコピーされたようで、どのファイルにも不要なAction + Reducerが多数ありました。このままではメンテナンス性の低い、メンテナンスしずらいプロジェクトになってしまいます。
リファクタリング作業では、
- Reactコンポーネントや
store/index.tsに書かれた型をtypes.tsファイルに移動（ない場合は作成） - 不要な型定義を削除
- 不要なAction + Reducer(Redux)を削除
- 複数の
types.tsで一致してない型定義を要素をマージして一致させる - 最終的に型定義を1つの
types.tsファイルにまとめました
規約違反
Ruby on Railsで有名になった設定より規約(CoC, convention over configuration)ですが、規約（ルール）を重視することでコードの理解し易さやさを高めメンテナンス性を上げることができます。
今回のコードには、
import ItemSearch from '../search/components/Search';
・・・
<Route path="/search_item" component={ItemSearch} />のように、export default Search;で公開されたコンポーネントを、importで違うコンポーネント名を付けているコードがあり最初は混乱しました。また、コンポーネント名がファイル名と同じでないコンポーネントもありました。😅
このような名前を適当に扱っているコードは、コードの理解し易さやさが下がり、最終的にはメンテナンス性を下げます。
他にもいくつかの規約や一般的なルールを守ってないコードがありました。
- 通常は関数内で使われない引数を
_または_からは始まる変数名で指定しますが、使われている引数が_（アンダーライン）から始まっている😅 - JSXが書かれていない、Reduxのファイルが
.tsxになっている😅
危ういコード
このプロジェクトではTypeScriptを使っていますが、やたらとany型が使われていました。例えば、
- 型定義があるのに使わずに
anyを指定 😅 - MUI(Material-UI)のonChengeの引数など調べにくい型を
anyを指定 😅 const y = x as any as Y_Type;のようにanyを使い強制的に型変換 😅
any型はJavaScriptからTypeScriptに移行するときや、とりあえず引数の型を調べるのが面倒な時には便利ですが、anyのまま残しておくとメンテナンス時には、この変数には何が入ってるだっけ？となりメンテナンス性を下げます。またas any asはTypeScriptの型の良さを全否定する危ない変換なので使わないようにするべきです。
つづく
今回はこれくらいで、たぶん続きを書きます。
