こんにちは！会員システムグループでエンジニアをしている山田です。
今回はNIFTY Tech Day 2023で掲示していたコードレビュー問題の解答編になります。
問題はこちらで公開していますので、まだ見てないよ！という方は是非チャレンジしてみてください。
出題内容
出題コードをおさらいしてみます。
AutoComplete.tsx
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 |
/* eslint-disable */ import React, { useEffect, useState } from 'react'; import { AutoCompleteList } from '../common/AutoCompleteList'; import { AutoCompleteContainer } from '../common/AutoCompleteContainer'; import { AutoCompleteItem } from '../common/AutoCompleteItem'; import StatefulTextBox from './StatefulTextBox'; const api = '/api/autoSuggest'; export const AutoComplete = () => { const [value, setValue] = useState(''); const [suggestion, setSuggestion] = useState<string[]>([]); useEffect(() => { const fetchFn = async () => { const response = await fetch(`${api}?q=${value}`); const responseJson = await response.json(); setSuggestion(responseJson.items); }; fetchFn(); }); const AutoCompleteWindow = () => { let listElements: JSX.Element[] = []; for (let suggestionItem of suggestion) { listElements.push( <AutoCompleteItem value={suggestionItem} onClick={setValue} /> ); } return <AutoCompleteList>{listElements}</AutoCompleteList>; }; return ( <AutoCompleteContainer> <StatefulTextBox onValueChange={(value) => setValue(value)} /> <AutoCompleteWindow /> </AutoCompleteContainer> ); }; |
StatefulTextBox.tsx
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 |
/* eslint-disable */ import { useEffect, useState } from 'react'; import { TextBox } from '../common/TextBox'; type Props = { onValueChange: (value: string) => void; }; export default function ({ onValueChange }: Props): JSX.Element { const [value, setValue] = useState(''); useEffect(() => { onValueChange(value); }, [value, onValueChange]); return <TextBox value={value} onChange={setValue} />; } |
これはテキストボックスへの入力値を元に、入力補完の機能(オートコンプリート)を実現するコンポーネントになっています。「common」からimportしているコンポーネントは既成のもの、という想定です。
テキストボックスの入力値をstateで保持するStatefulTextBoxがあり、その値変化をAutoCompleteコンポーネントで受け取って、useEffectで補完内容をfetchし、表示するという流れです。
一見問題なく動くように見えますが、大きく区分すると以下のような問題・改善点を抱えています。
- バグ挙動
- エラー処理・キャンセルの未考慮
- パフォーマンス上の問題
- コードの統一性の問題
- 設計上の改善点
- 仕様上の改善点
バグ挙動
無限レンダリング
AutoCompleteコンポーネントではuseEffectの処理の中でsetSuggestion関数を呼んでいます。これによりstateが更新されますが、これは再レンダリングを引き起こすため、再びuseEffectが呼ばれることになります。つまり、このコンポーネントは無限にレンダリングが走り、無限にfetchを呼ぶことになります。
useEffectを使用する場合は第二引数(dependencies)でスキップ条件を記載するのが適切です。
|
1 2 3 |
useEffect(() => { ... }, [value, setSuggestion]); |
一部文字列での補完不具合
fetchをする際に文字列を直接URLに埋め込んでしまっているため、URLで使用する文字列では不具合を起こします。例えば「a&b」など「&」を含む入力を渡した場合、「&」以降の文字列が別パラメータになってしまうため、補完に使われなくなってしまいます。
URLに渡すパラメータは適切にURLエンコードをすべきです。要件によってはさらにサニタイズ・ノーマライズを行ったほうが良いでしょう。
|
1 2 |
const params = new URLSearchParams({ q: value }); const response = await fetch(`${api}?${params.toString()}`); |
補完内容がなくても枠が表示される
補完内容を表示しているモーダル部分は、中身が0件でも表示されてしまいます。このため、無駄な枠が表示されたままになってしまいます。
補完内容がない場合は非表示にしておくのが良いでしょう。
|
1 2 3 4 5 6 7 8 |
return ( <AutoCompleteContainer> ... { suggestionData.length !== 0 && ( ... ) } </AutoCompleteContainer> ); |
エラー処理・キャンセルの未考慮
fetch時の異常系未考慮
fetchを実行する際に異常系の考慮をしていないため、以下の問題を抱えています。
- fetchが例外を返した場合にクラッシュする
- 404、500などの異常系応答を返した場合に異常値が入る
- 200応答でも想定外の値が返ってきた場合、異常値が入る
2つ目に関して、fetchは異常系応答でも例外を返さないという特徴があるので注意が必要です。
fetchを利用する際はレスポンスの検証とエラーハンドリングを行いましょう。
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 |
const ResponseSchema = z.object({ items: z.array(z.string()); }); ... try { const response = await fetch(...); if (!response.ok) { // 異常系応答時のハンドリング } const responseJson = await response.json(); // zodでのバリデーション const parsedResponse = ResponseSchema.parse(responseJson); ... } catch (e) { // 例外ハンドリング } |
上記例ではバリデーションライブラリとしてzodを利用し、レスポンスのJSONオブジェクトが期待した型であるかどうかをチェックしています。
異常系応答に関しては、kyやaxiosなどのサードパーティHTTPクライアントを利用することで、例外として処理するようにすることもできます。
非同期処理のキャンセル
useEffectでは非同期関数を実行していますが、これは完了前にコンポーネントが再レンダリングされたとしても実行され続けます。これはメモリリークやRace Conditionを引き起こす問題になります。
fetchはAbortControllerを利用することでキャンセル処理が可能です。ただしSafari12.0以下など、古いブラウザでは対応していないことには注意が必要です。
|
1 2 3 4 5 6 7 8 9 |
useEffect(() => { const abortController = new AbortController(); const response = await fetch(..., { signal: abortController.signal }); ... return () => { abortController.abort(); }; }); |
ただしこの場合でも、後続のJSONパース処理などはキャンセルできません。厳密にRace Conditionを防ぐのであれば、JSONのパースを同期処理にする、フラグで制御するようにするなどが必要になってくるでしょう。
そもそもuseEffectを使わず、SWRやTanstack Queryなどを使うようにするという方法もあります。
パフォーマンス上の問題
コンポーネント内でのコンポーネント定義
AutoCompleteコンポーネント内でAutoCompleteWindowコンポーネントを作っています。AutoCompleteが再レンダリングされると、AutoCompleteWindowは再度定義されることになり、以前のものとは別のコンポーネントになります。したがって毎回再生成されてしまうので、パフォーマンス上不利になります。また今後AutoCompleteWindowが拡張されてstateやフォーカスを持つようになった場合、再レンダリングごとに状態がリセットされてしまうことにもなります。
このため、コンポーネント内でコンポーネントを定義してはいけません。別コンポーネントとして切り出すか、今回のような規模であれば同じreturnの中でまとめてしまって良いかもしれません。
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 |
return ( <AutoCompleteContainer> ... <AutoCompleteList> {suggestionData.map((suggestion) => ( <AutoCompleteItem value={suggestion} onClick={setValue} key={suggestion} /> ))} </AutoCompleteList> </AutoCompleteContainer> ); |
forでコンポーネントを作るのはReactではあまり一般的ではないので、上記例ではmapを利用した形に直しています。
stateが二重管理になっている
テキストボックスへの入力値をAutoCompleteとTextBoxの両方でstateとして保持しています。
このような場合は上位コンポーネントに状態を移動(リフトアップ)するのが良いでしょう。今回の場合はStatefulTextBoxコンポーネントがそもそも不要で、AutoCompleteからTextBoxを直接呼ぶような形で良いかもしれません。
コードの統一性の問題
ESLintを無効化している
両ファイルとも、冒頭でESLintを完全に無効化してしまっています。ESLintを導入しているということはチーム内で守るべきルールが定められているということなので、これを無視してはいけません。実際、本コードの問題の多くはESLintによって指摘されます。
ESLintには従うことが原則であり、例外的に無視せざるを得ない箇所は理由を添えておくのが良いでしょう。無視コメントが増えるようなら、ESLintの設定を見直すときかもしれません。
|
1 2 |
// <無視する理由> // eslint-disable-next-line <無視ルール> |
コンポーネント定義方法に違いがある
AutoCompleteとStatefulTextBoxでは、コンポーネント定義に以下の違いがあります。
- named exportかdefault exportか
- 関数定義がアロー関数かfunctionか
- 戻り値型(JSX.Element)を明示するかしないか
それぞれどちらを使うかはチームの方針によりますが、同一プロジェクト内で分かれていると不要な混乱を呼びます。特にexportの方法が異なると、importする際にどちらを使うべきかを調べる必要が生まれるので、どちらかに統一されているべきです。
また変数名を省略したdefault export(anonymous default export)が使われていますが、これは以下のような問題を引き起こすので、使用しないほうが良いです。
- React Developer Toolでコンポーネント名が「default」になる
- IDE上での自動importが効かない
- デバッグ実行でのFast Refreshが動作しない
default exportを使う場合は変数名をつけてからexportするようにしましょう。
|
1 2 |
const StatefulTextBox = ... export default StatefulTextBox; |
設計上の改善点
HTTPアクセス部分の分離
HTTPアクセス部分がコンポーネントにベタ書きなので、ユニットテストがしづらい状態になっています。
mswなどのHTTPモックを使うという手もありますが、別関数に分離することでテストしやすくなる可能性があります。HTTPクライアントが変わったり、API仕様が変わった時にも対応しやすいでしょう。
|
1 2 3 4 5 6 |
const getSuggestion = async (query: string): Promise<string[]> => { const response = fetch(...); ... return suggestion } |
補完ロジックの分離
テスト設計方針にもよりますが、補完部分のロジックをカスタムフックに分離することで、ロジック部分のみでテストできるようにしたほうが良いかもしれません。
|
1 2 3 4 5 6 7 |
const useSuggestion = (value: string): string[] => { const [suggestion, setSuggestion] = useState<string[]>([]); useEffect(() => { ... }); } |
仕様上の改善点
これらは入力補完というものに固有の問題で、気づきにくいものです。当日のオフライン会場でも、ヒントなしでこの可能性に気付かれた方は0名でした。
仕様にも関わるのでプロダクトオーナーへの確認も必要となるでしょう。
空文字列に対する補完は不要
入力補完は入力された文字列を元に候補を表示するので、文字列が空の場合は候補を出すことは通常困難であるはずです。
API側の仕様確認が必要ですが、空文字列の場合に補完を停止することで不要なアクセスを抑えることができます。
|
1 2 3 4 5 |
useEffect(() => { if (value !== '') { ... } }); |
入力のたびに補完は不要
入力文字1文字1文字に対してまで補完を実行するのはほとんどの場合過剰であるはずです。ある程度の文字を打ち込み、入力が止まったタイミングで表示するような挙動で問題ないでしょう。
以下のような遅延(debounce)の処理を入れることで、ユーザが連続入力している間は補完処理を止めることができるようになります。こうすることでAPI側の負荷を大きく下げることができます。
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 |
const useDebounce = <T>(value: T, delayMs?: number): T => { const [debouncedValue, setDebouncedValue] = useState<T>(value); useEffect(() => { const timer = setTimeout(() => { setDebouncedValue(value); }, delayMs ?? 500); return () => { clearTimeout(timer); }; }, [value, delayMs]); return debouncedValue; }; const [value, setValue] = useState(""); // debounceした値を補完のクエリとして使う const debouncedValue = useDebounce(value); |
ただしこれは補完結果の表示が遅れることに繋がるので、チーム内で仕様調整が必要になるでしょう。
その他の改善点など
Reactのimportは不要
昔はJSX記法を利用した場合、トランスパイル時に React.createElement()に変換される仕様だったため、必ず import React from 'react';の記載が必要でした。
React 17以降はJSX Transformと呼ばれる形の変換に変更となり、 import ... from 'react/jsx-runtime';の形式のimport文が自動で挿入されるようになったので、この記載は不要になりました。
変数・定数の命名
変数名を具体的にしたほうがわかりやすい箇所があります。
例えば変数「api」が挿すのはURLなので、固定値であることも含めると
|
1 |
const SUGGEST_API_URL = "/api/autoSuggest"; |
のようにしたほうがわかりやすくなります。
コンポーネント名
事前に想定はしていなかったのですが、何名かの方から「AutoComplete」というコンポーネント名を変えた方が良いのでは、というレビューをいただきました。
一般的にコンポーネント名は名詞ですが、AutoCompleteは動詞です。またAutoCompleteという名前は機能を表しているので、「テキストボックスと補完ウィンドウのセット」というUIのイメージが湧きづらい命名になっています。
MUIなどのコンポーネントライブラリでは「Autocomplete」の名称で提供されているので一概には言えませんが、これもより具体的な名前に変更した方がわかりやすくなる箇所でしょう。
絶対importか相対importか
これも事前に想定はしていなかったのですが、何名かの方から「絶対importの方が良いのでは？」というレビューをいただきました。このような形のものですね。
|
1 2 3 4 |
// 絶対import import { AutoCompleteList } from 'components/common/AutoCompleteList'; // エイリアスを利用した絶対import import { AutoCompleteList } from '@/components/common/AutoCompleteList'; |
相対パスだとディレクトリ構造がわかりにくいので、絶対パスにした方が可視性が高い、という利点があります。もちろんプロジェクト内で統一する必要はあります。
ただし今後やってくるであろうES Modulesへの対応では注意が必要です。
CommonJSと異なり、ES Modulesの仕様では絶対パスという仕様がありません。トランスパイル先をES Modulesにした場合には、すべて相対パスで記載することが基本になります。webpackなどのバンドラーが変換してくれることもありますが、それらの設定を再確認する必要があります。
以上、それほど長くはないコードだったのですが、よく読むと問題があるようなコードになっていました。
当日の様子
当日はこのような形で掲示していました。
初の試みで参加いただけるか不安もあったのですが、多くの方にレビューをいただき、
- useEffect()の第二引数忘れてる
- ロジックは分離して単体テストできるようにした方が良さそう
- APIのURLはベタ書きしちゃってもいいのでは
- (上コメントに対して)いやそれはどうだろう…
- 関数の改行が見づらい
など、多くのレビューをいただくことができました。
まとめ
問題点に気づくことができましたでしょうか。コードレビューに正解はありませんので、記載した方法以外のやり方があるかもしれませんし、それ以外の場所にも何かが眠っているかもしれません。私はこう思うよ！というようなものがありましたら、ぜひX(Twitter)などで教えていただければ幸いです。
このような取り組みはニフティとしても初めてだったのですが、参加いただいた方には概ね好評だったようで喜ばしく思っています。一方でフロントエンド領域、かつReactに強く依存した問題でしたので、バックエンドエンジニアの方ですと参加が難しくなってしまった、というのが反省点です。
次回も同様の問題をお見せできるようなことがあれば、Pythonなどの問題も用意できるようにしていきたいと思っておりますので、今回参加できなかった方もぜひ挑戦いただければ幸いです。
ニフティでは、
さまざまなプロダクトへ挑戦する
エンジニアを絶賛募集中です！
ご興味のある方は以下の採用サイトより
お気軽にご連絡ください！
ニフティに興味をお持ちの方は
キャリア登録をぜひお願いいたします！
connpassでニフティグループに
参加いただくと
イベントの
お知らせが届きます！
