去年の自分のコードをレビューしてみた

PlayGround -

こんにちは、大学3年生のRyomaです。

ふと、「1年前に自分が書いたコードってどんな感じだったっけ？」と思い立ち、当時開発したTodoアプリのリポジトリを掘り起こしてみました。

アプリのスクリーンショット

このアプリは、私が所属していた旧フロントエンドコースの課題として作成したものです。
当時は「動けば正義！」「Figmaのデザイン通りに実装できてすごい！」と達成感に浸っていたのですが、今の視点で見返すと……

これ、直したい。

と思う箇所が次々と出てきました。
今回は、過去の自分へのコードレビューを通して、今の自分ならどう実装するか、という改善提案をまとめてみたいと思います。

1年前のコードの「ここが気になる」

このアプリは React + styled-components で書かれたシンプルなTodoアプリです。
Atomic Designを採用し、ディレクトリ構成もしっかりしているように見えますが、中身を見ると"動くこと"だけを優先した実装がありました。

  1. UIとロジックが密結合: コンポーネントの中に useState や複雑な関数が詰め込まれている。
  2. Reactのアンチパターン: 無理やりDOMを操作している箇所がある。
  3. テストがない: リファクタリングしようにも、壊れないか不安。

これらに対して、現在の自分の知識でこう直すべきという提案をしていきます。

1. UIとロジックの分離 (Custom Hook)

一番気になったのは、メインのコンポーネントである TodoCard に、状態管理のロジックがベタ書きされていたことです。

Before: 1年前のコード

// components/Organisms/TodoCard/index.jsx
const TodoCard = ({ tasks = [] }) => {
  const [taskList, setTaskList] = useState(tasks);

  // UIの中にロジックが混在していて見通しが悪い...
  const handleTaskNameChange = (value, index) => {
    if (value.trim() === "") {
      // ...削除処理
    } else {
      // ...更新処理
    }
  };

  return <StyledWrapper>{/* ...JSXの記述 */}</StyledWrapper>;
};

これだと、デザインを修正したいだけなのにロジックを誤って壊してしまうリスクがあります。また、ロジック単体のテストも難しいと思います。

Proposal: Custom Hookへの切り出し

現在は、UIとロジックを分離するという考え方が一般的になっています。もし今実装するなら、ロジック部分を useTodo というカスタムフックに抽出します。

// hooks/useTodo.js
export const useTodo = (initialTasks = []) => {
  const [taskList, setTaskList] = useState(initialTasks);

  const addTask = () => { ... };
  const updateTaskName = (index, value) => { ... };

  // ロジックと状態だけを返す
  return { taskList, addTask, updateTaskName };
};

これを使うと、コンポーネント側はスッキリします。

// components/Organisms/TodoCard/index.jsx
const TodoCard = ({ tasks = [] }) => {
  // ロジックはフックから呼ぶだけ！
  const { taskList, addTask, updateTaskName } = useTodo(tasks);

  return <StyledWrapper>{/* UIのレンダリングに集中できる */}</StyledWrapper>;
};

2. Controlled Component

次に気になったのが、入力フォームの実装です。
1年前の私は、Reactの流儀がいまいち分かっておらず、useEffect と useRef を使って無理やり値を書き換えていました。

Before: DOMを直接操作

// components/Atoms/Input/index.jsx
useEffect(() => {
  if (inputRef.current) {
    // Reactの管理外でDOMを直接書き換えている
    inputRef.current.value = defaultValue;
  }
}, [defaultValue]);

これは予期せぬバグの温床になります。今なら、Reactが推奨する Controlled Componentとして実装します。

Proposal: Reactに任せる

const Input = ({ value, onChange }) => {
  return (
    <StyledInput
      value={value} // ReactのStateを正とする
      onChange={(e) => onChange(e.target.value)}
    />
  );
};

DOMを直接触らないというのはReactの基本なので、当時は動けばいいで突っ走っていたことがわかります。

3. テストコードを書く

1年前は「テストコード？ 何それおいしいの？」状態でしたが、今はテストがないリファクタリングはまずいと自然に考えるようになりました。

ロジックを useTodo に切り出す提案をしましたが、これによってUIのレンダリングを気にせず、ロジック単体のテストが書けるようになります。

Proposal: ロジックのテスト

// hooks/useTodo.test.js
test("タスクが追加できること", () => {
  const { result } = renderHook(() => useTodo());

  act(() => {
    result.current.addTask();
  });

  expect(result.current.taskList).toHaveLength(1);
});

このように書いたコードの正しさを自動で確認できることも重要だと感じています。

最近はAIを補助的に使う機会が増えました。時短になるし、何より楽なので。
しかし、AIが書いたコードが本当に正しいか？を判断するのは、最終的には人間の責任です。AI生成コードの品質を担保するためにも、テストコードを用意しておくと安心だと思います。

まとめ

今回のレビューを通して、以下の変化を感じました。

  • 1年前: どうやって実装するかに必死だった。
  • 現在: どう設計すれば壊れにくいかを考えられるようになった。

コードが動くのは当たり前で、その先の読みやすさや直しやすさを意識できるようになったのが、この1年間の最大の成長かもしれません。

過去のコードは恥ずかしいものですが、それは成長の証でもあると思います！皆さんも、昔の自分のコードをレビューしてみてはいかがでしょうか？きっと新しい発見があるはずです。