はじめに
こんにちは、koyablueです。
この前久々にリーダブルコードを読み直しました。
リーダブルコードの内容をルール化してAIに任せるといい感じに適用できるのでは？と思ったのでやってみます。
具体的なコーディング規約準拠かどうか、というルールを適用させている例はわりとある気がしますが、可読性を担保するための汎用的なルールを正しく適用することは可能なのでしょうか（結果はタイトル通りなわけですが）。
やること
- リーダブルコードからルールにできそうなものを地道に抽出し、マークダウンにする
- ルールのマークダウンをCursorのProject Rulesに設定する
- Cursorに「ルールに従ってコードレビューしてください」と投げてレビューさせてみる
リーダブルコードから使えそうなルールを抜き出したものがこちらです
抽出したルール
# 理解しやすいコードの前提
- 他の人が最短時間で理解できる
- 簡潔さやコードの小ささより、可読性や理解容易性が重視されている
- 実行の流れが追いやすい
# 命名
- 具体的で明確な名前を使用する
- スコープが小さい場合は汎用的な命名も可
- 必要であれば接尾辞や接頭辞で情報を追加する
- 長い名前を許容する
# コードのレイアウト
- コードは意味のある並びで整列、改行、段落分けを行い、一貫性を保つ
- 似ているコードはできるだけ書き方を統一し、視覚的に似ているように見せる
- 関連するコードをまとめてブロックにする
# コメント
- 必要であれば、以下の部分について正確に簡潔にコメントを書く
- そのコードがそうなっている理由や背景
- クラスや関数の全体像について
- コードを読んだ人に質問されそうなこと
- ハマりそうな罠
- コードの欠陥
- 必要に応じて以下のプレフィクスを用いる
- TODO
- 後で手を付ける
- FIXME
- 既知の不具合がある
- 以下の場合はコメントを書かない
- 自明な内容
- 悪い実装をそのままにしてコメントで補う場合
# ループ
- 可能な限り単純化する
# 条件式
- 可能な限り単純化する
## 条件式の引数の並び順
- 左側に調査対象の式を書く。右側に比較対象の式を書く
```
推奨例: if (length >= 10)
非推奨例: if (10 <= length)
```
## 条件の書き方
- 否定形よりも肯定形を使用する
- 推奨例: `if (hasData)`
- 非推奨例: `if (!hasData)`
## if-elseブロックの並び順
- 単純な条件を先に書く
- 関心を引く条件や目立つ条件を先に書く
## 三項演算子
- 三項演算子によってif-elseで書くよりも簡潔になる場合のみ使用する
- 行数を短くするためだけの目的で使用しない
- 三項演算子を使用した結果理解に時間がかかるコードになる場合、使用しない
## early return、ガード節
- 関数からearly returnできる場合はearly returnする
# コードのネスト
- ネストはできる限り浅くする
# 巨大な式は分割する
- 説明のために一時変数を用いる
- 要約のために一時変数を用いる
- ド・モルガンの法則を使用する
- 短絡評価はそれにより簡潔になる場合のみ使用する。可読性が下がる場合は使用しない
# 変数
- 不要な変数を作らない
- 変数のスコープはできる限り小さく保つ
- 変数は出る限りイミュータブルに保つ
# 汎用コードの分離
- プロジェクト固有のコードから汎用コードを分離する
# １つのコードブロックは一度に１つのタスクを実行する
- 複数の種類の処理を混在させない
# ライブラリ・ツール
- 既存のライブラリやツールで代替できる部分はそれらを使用する
# テスト
- 目的、前提、期待結果が理解できる
- 不要な詳細をテスト本体に露出させない
- テスト失敗時に問題の特定に役立つエラーメッセージが出力されるか
- テストに使用する関数名には、テストの対象と状況が明確にわかる名前をつける
- テストを追加、変更しやすい構造を保つ
- 機能ごとに小さなテストケースに分割する
- 必要に応じてテスト用にヘルパー関数を定義する
- 100%カバレッジを目的化しない。コストと価値のバランスを意識する。
- 低リスクな領域のテストを無理に書かない
## テスタビリティ
- 関数・クラスは内部状態や依存が少なく、単体で検証しやすい状態にする
- 外部依存を最小限に抑える
- インターフェースをシンプルかつ明確にする
- テストのために実装側の可読性・設計を犠牲にしない
さらにこれをルールとして適用しやすい形に取捨選択します
ルール化したもの
以下のルールに従ってコードレビューしてください。
# Basic Principles
- MUST 可読性や理解容易性を優先する
- MUST 実行フローを追いやすく保つ
# Naming
- MUST 具体的で明確な名前を使用する
- SHOULD 必要であれば接尾辞や接頭辞で情報を追加する (例: isEnabled, weightKg)
# Layout
- MUST 関連コード間を1行空けて論理ブロックを区切る
- MUST 類似コードを視覚的にそろえ、差分を浮かび上がらせる
# Control Flow
- MUST ループや条件のネストが可読性を損なう場合は、関数抽出・ガード節などで浅く保つ
- MUST 三項演算子は if‑else より 可読性が高くなる場合のみ使用する
# Conditionals
- MUST 調査対象を左辺、比較対象を右辺に置く
```
推奨例: if (length >= 10)
非推奨例: if (10 <= length)
```
- MUST 肯定形を優先する
```
推奨例: if (hasData)
非推奨例: if (!hasData)
```
# Comments
- SHOULD 「何をするか」より なぜそうするか を説明する。
- MUST 作業途中は TODO:、既知バグは FIXME: プレフィクスを付ける
- MUST NOT 自明なコードや悪実装の言い訳を書く
# Variables
- MUST スコープを最小化する
- SHOULD const / readonly を優先しイミュータブルに保つ
- MUST NOT 不要な一時変数を生成する
# Splitting Large Expressions
- SHOULD 意味付けの一時変数で式を分割し、可読性を高める
- SHOULD ド・モルガンの法則を用いて否定条件を簡潔にする
- MUST NOT 複雑な短絡評価で可読性を落とす
# Modularization
- MUST プロジェクト固有ロジックと汎用ロジックをモジュールで分離する
# Tests
- MUST 目的、前提、期待結果が理解できる
- MUST コストと価値を考慮してテスト範囲を決定する
- SHOULD 失敗時に原因特定しやすいエラーメッセージを出力する
- SHOULD テストは小さく独立させ、ヘルパーで重複を排除する
- SHOULD NOT 100%カバレッジを目的化する
# Testability
- MUST 関数やクラスは副作用・外部依存を最小化する
- MUST インターフェースをシンプルかつ明確に保つ
- MUST NOT テストの都合で実装の設計・可読性を著しく犠牲にする
ルールに従ってレビューさせてみる
モデルの設定は以下の通りです。モデルの選択はAutoにしてCursorに任せます
今回レビューしてもらうフォーム
まずはルール違反しまくりのフォームをレビューしてもらう
とんでもない実装をレビューさせてみます
最悪な実装
import React, { useState } from "react";
import { useForm } from "react-hook-form";
import { z } from "zod";
import { zodResolver } from "@hookform/resolvers/zod";
const a = z.object({
p: z.string().min(1).max(50),
c: z.string().min(1).max(50),
l: z.string().min(1),
s: z.string().regex(/^\d+\s*(万円\/月|万円\/年)$/),
d: z.string().min(1).max(1000),
});
type A = z.infer<typeof a>;
type B = {
position: string;
companyName: string;
location: string;
salary: { amount: number; unit: "month" | "year" };
description: string;
};
export const MessedUpJobForm: React.FC = () => {
const [x, y] = useState<A | null>(null);
const { register, handleSubmit, watch, formState } = useForm<A>({
resolver: zodResolver(a),
});
const b = (data: A) => {
y(data);
};
const d = () => {
if (!x) {
return;
}
const m = x.s.match(/^(\d+)\s*万円\/(月|年)$/);
const t = {
position: x.p,
companyName: x.c,
location: x.l,
salary: {
amount: Number(m?.[1]),
unit: m?.[2] === "月" ? "month" : "year",
},
description: x.d,
};
console.log("送信", t);
alert("おくった");
y(null);
};
return x ? (
<div className="max-w-2xl mx-auto p-4">
<h2>確認</h2>
<div>{x.p}</div>
<div>{x.c}</div>
<div>{x.l}</div>
<div>{x.s}</div>
<div>{x.d}</div>
<div>
<button
type="button"
onClick={() => {
y(null);
}}
className="mr-2"
>
もどる
</button>
<button type="button" onClick={d}>
送信
</button>
</div>
</div>
) : (
<form onSubmit={handleSubmit(b)} className="max-w-2xl mx-auto p-4">
<div>
<label>職種</label>
<input {...register("p")} />
</div>
<div>
<label>会社名</label>
<input {...register("c")} />
</div>
<div>
<label>勤務地</label>
<input {...register("l")} />
</div>
<div>
<label>給与</label>
<input {...register("s")} placeholder="例: 30 万円/月" />
</div>
<div>
<label>業務</label>
<textarea {...register("d")} rows={4} />
</div>
<button type="submit">送信</button>
</form>
);
};
出力されたレビューがこちら
レビュー結果
以下、ルールに従ったコードレビューです。
---
# 可読性・理解容易性
- 変数名や関数名が短すぎて意味が伝わりにくいです（例: a, b, d, x, y）。具体的で明確な名前を使用してください。
- 型 B は定義されていますが、使われていません（未使用警告あり）。不要なら削除しましょう。
---
# 命名
- a → jobSchema など、スキーマの内容が分かる名前に。
- A → JobFormInput など、型の意味が分かる名前に。
- b → onFormSubmit など、役割が分かる名前に。
- d → handleConfirmSubmit など、何をする関数か分かる名前に。
- x, y → formData, setFormData など、状態の意味が分かる名前に。
---
# レイアウト・論理ブロック
- 関連するコード間に空行を入れて、論理ブロックを明確に区切るとより良いです。
---
# コントロールフロー・条件分岐
- ネストは浅く保たれており、ガード節も使われていて良いです。
- 条件式の左辺・右辺もルール通りです。
---
# コメント
- コメントはありませんが、現状のコードは自明なので不要です。
ただし、なぜこのような変換（給与の正規表現分解など）をしているかは、**なぜそうするか**のコメントがあると親切です。
---
# 変数・スコープ
- watch, formState は使われていないので削除しましょう（未使用警告あり）。
- 不要な一時変数はありませんが、m も salaryMatch など意味のある名前にしましょう。
---
# その他
- const を適切に使っており、イミュータブル性も保たれています。
- JSX内のラベルやボタン名も分かりやすいです。
---
# まとめ（修正提案）
- 変数・関数・型の命名を具体的に
- 未使用の型や変数の削除
- 必要に応じて「なぜ」そうするかのコメント追加
---
ご参考までに、命名例を一部挙げます：
```typescript
const jobSchema = z.object({ ... });
type JobFormInput = z.infer<typeof jobSchema>;
const [formData, setFormData] = useState<JobFormInput | null>(null);
const onFormSubmit = (data: JobFormInput) => { setFormData(data); };
const handleConfirmSubmit = () => { ... };
```
ご不明点や追加レビュー希望箇所があればご指示ください
すばらしいですね。いい感じです。
「それはそう」という当たり前のコメントだとも言えますが、今回の試みでは当たり前を徹底してもらうことに意味があります。
では次の試みに移ります。
微妙なコードに対するレビュー
では「ダメだけど最悪ではない」みたいなコードに対してはどうでしょうか。
こういう微妙な実装に対してちゃんとレビューできてほしいです。
最悪ではない実装
import React, { useState } from "react";
import { useForm } from "react-hook-form";
import { z } from "zod";
import { zodResolver } from "@hookform/resolvers/zod";
const validation = z.object({
position: z
.string()
.min(1, "募集職種は必須です")
.max(50, "50文字以内で入力してください"),
companyName: z
.string()
.min(1, "会社名は必須です")
.max(50, "50文字以内で入力してください"),
location: z.string().min(1, "勤務地は必須です"),
salary: z
.string()
.regex(
/^\d+\s*(万円\/月|万円\/年)$/,
"給与は「半角数字 + 万円/月・万円/年」の形式で入力してください"
),
description: z
.string()
.min(1, "業務内容は必須です")
.max(1000, "1000文字以内で入力してください"),
});
// フォームの型
type FormValues = z.infer<typeof validation>;
// API送信用の型
type Request = {
position: string;
companyName: string;
location: string;
salary: {
amount: number;
unit: "month" | "year";
};
description: string;
};
export const OKJobForm: React.FC = () => {
const [preview, setPreview] = useState<FormValues | null>(null);
const {
register,
handleSubmit,
reset,
formState: { errors },
} = useForm<FormValues>({
resolver: zodResolver(validation),
});
const onSubmit = (data: FormValues) => {
setPreview(data);
};
const onConfirm = () => {
if (preview !== null) {
const salaryMatch = preview.salary.match(/^(\d+)\s*万円\/(月|年)$/);
const requestBody: Request = {
position: preview.position,
companyName: preview.companyName,
location: preview.location,
salary: {
amount: Number(salaryMatch?.[1] || 0),
unit: salaryMatch?.[2] === "月" ? "month" : "year",
},
description: preview.description,
};
console.log("送信データ:", requestBody);
alert("送信完了！(コンソールを見てください)");
setPreview(null);
reset();
} else {
return;
}
};
if (preview === null) {
return (
<form
onSubmit={handleSubmit(onSubmit)}
className="max-w-2xl mx-auto p-6 space-y-4"
>
<div>
<label className="block mb-1 font-bold">募集職種</label>
<input
{...register("position")}
className="border w-full p-2 rounded"
/>
{errors.position && (
<p className="text-red-500 text-sm">{errors.position.message}</p>
)}
</div>
<div>
<label className="block mb-1 font-bold">会社名</label>
<input
{...register("companyName")}
className="border w-full p-2 rounded"
/>
{errors.companyName && (
<p className="text-red-500 text-sm">{errors.companyName.message}</p>
)}
</div>
<div>
<label className="block mb-1 font-bold">勤務地</label>
<input
{...register("location")}
className="border w-full p-2 rounded"
/>
{errors.location && (
<p className="text-red-500 text-sm">{errors.location.message}</p>
)}
</div>
<div>
<label className="block mb-1 font-bold">給与</label>
<input
{...register("salary")}
placeholder="例: 30 万円/月"
className="border w-full p-2 rounded"
/>
{errors.salary && (
<p className="text-red-500 text-sm">{errors.salary.message}</p>
)}
</div>
<div>
<label className="block mb-1 font-bold">業務内容</label>
<textarea
{...register("description")}
rows={6}
className="border w-full p-2 rounded"
/>
{errors.description && (
<p className="text-red-500 text-sm">{errors.description.message}</p>
)}
</div>
<button
type="submit"
className="mt-6 px-4 py-2 bg-blue-500 text-white rounded"
>
確認画面へ
</button>
</form>
);
} else if (preview !== null) {
return (
<div className="max-w-2xl mx-auto p-6 space-y-4">
<h2 className="text-2xl font-bold">確認画面</h2>
<div>
<strong>募集職種:</strong> {preview.position}
</div>
<div>
<strong>会社名:</strong> {preview.companyName}
</div>
<div>
<strong>勤務地:</strong> {preview.location}
</div>
<div>
<strong>給与:</strong> {preview.salary}
</div>
<div>
<strong>業務内容:</strong>
<br />
<div className="whitespace-pre-line">{preview.description}</div>
</div>
<div className="flex gap-4 mt-4">
<button
type="button"
onClick={() => setPreview(null)}
className="px-4 py-2 bg-gray-300 rounded"
>
戻る
</button>
<button
type="button"
onClick={onConfirm}
className="px-4 py-2 bg-blue-500 text-white rounded"
>
送信
</button>
</div>
</div>
);
}
};
少なくとも命名、if文の記述方法などに対してはちゃんとコメントしてもらいたいです。
ではレビューさせてみます。
レビュー結果
# 可読性・理解容易性
- 変数名・関数名・型名が具体的かつ明確で、役割が一目で分かります。
- 実行フローも追いやすく、フォーム送信→確認→送信の流れが明確です。
---
# 命名
- validation, FormValues, Request, onSubmit, onConfirm, preview, setPreview など、意味が明確な命名ができています。
- 型名も用途が分かりやすいです。
---
# レイアウト・論理ブロック
- 関連するコード間に空行があり、論理ブロックが明確です。
- JSX内の各フォーム要素も適切に分割され、視認性が高いです。
---
# コントロールフロー・条件分岐
- ネストは浅く、ガード節も適切に使われています。
- 条件式も肯定形・左辺調査対象・右辺比較対象のルールを守っています。
---
# コメント
- コメントは最小限ですが、型の説明やバリデーションの意図が明確なので問題ありません。
- 必要に応じて「なぜ」そうするかのコメントを追加しても良いですが、現状は自明です。
---
# 変数・スコープ
- 変数のスコープは最小化されており、不要な一時変数もありません。
- const/letの使い分けも適切です。
---
# その他
- バリデーションエラーの表示も分かりやすく、ユーザー体験に配慮されています。
- UIクラス名も一貫性があり、可読性が高いです。
- JSXの条件分岐もシンプルで、可読性を損なっていません。
---
# 改善点（あえて挙げるなら）
- salaryMatch?.[1] || 0 で 0 をデフォルトにしていますが、バリデーションで必ずマッチするため、salaryMatch が null になることは基本ありません。型安全のための保険としてはOKですが、コメントで意図を補足しても良いでしょう。
- 送信後に reset() でフォームを初期化していますが、reset() の引数で初期値を明示しても良いです（現状でも問題なし）。
---
# まとめ
- ルールに沿った非常に良い実装です。
- 命名・レイアウト・コントロールフロー・バリデーション・UI いずれも高水準です。
- 小さな改善余地はあるものの、現状でも十分に読みやすく保守しやすいコードです。
変数名・関数名・型名が具体的かつ明確で、役割が一目で分かります。
そう？ だいぶ汎用的な命名に見えるけど...
ネストは浅く、ガード節も適切に使われています。
ほんとに？ ガード節使ってなくない？
JSXの条件分岐もシンプルで、可読性を損なっていません。
そう言い張るのは無理があると思う
全然ちゃんとレビューできていませんね。
ルールに改善の余地がありそうなので、質問してみます。
なぜレビューの質が悪いのか質問
レビューありがとうございます。レビューが間違っています。
以下はあなたが見逃した部分の例です
- onConfirmでは早期リターンが可能です。無駄な条件文を書いています
- リターンするJSXも早期リターンが可能です。無駄な条件文を書いています
- モジュール分割可能な部分がいくつもあります
- 命名をもっと具体的にできます
これらを踏まえて質問です:
- なぜルールに準拠しない部分をたくさん見逃したのですか？
- レビュー精度を上げるにはどのようにルールを改善すればいいですか？
- レビュー精度を上げるにはどのように質問を改善すればいいですか？
質問に対する答え
ご指摘ありがとうございます。
ご提示いただいた見逃しポイントは、確かにルール（特に「ガード節による早期リターン」「モジュール分割」「より具体的な命名」）に照らしても重要な観点です。
以下、ご質問に順にお答えします。
1. なぜルールに準拠しない部分をたくさん見逃したのですか？
AIの限界:
コード全体の流れや構造を一度に把握し、すべてのルール違反を網羅的に指摘するのは難しい場合があります。特に、明示的なバグや大きな可読性の問題がない場合、細かな改善点（早期リターンやモジュール分割、命名のさらなる具体化など）は見落としやすいです。
ルールの適用範囲の曖昧さ:
ルールが「MUST」「SHOULD」などで記載されていても、どの程度まで厳密に適用するかの判断がAI側で曖昧になることがあります。
例: 「早期リターンを使うべき」か「現状でも十分可読性が高い」とみなすか。
レビュー観点の優先順位:
明らかなバグやアンチパターンがない場合、AIは「現状でも十分良い」と判断し、より細かな改善提案を省略することがあります。
2. レビュー精度を上げるにはどのようにルールを改善すればいいですか？
具体的なアンチパターン例・推奨例を追加する
例:
「if (x) { ... } else { return; }」→「if (!x) return; ...」のようなガード節の推奨例・非推奨例を明記
「複数のreturnでJSXを分岐する例」「条件分岐でJSXを返す例」など、具体的なコード例をルールに追加
「MUST」「SHOULD」の基準を明確化する
どの程度まで厳密に守るべきか、例外はどんな場合かを明記する
「見逃しやすいポイント」リストをルール末尾に追加する
例:
早期リターンの有無
JSXの条件分岐の簡素化
モジュール分割の余地
命名の具体性
3. レビュー精度を上げるにはどのように質問を改善すればいいですか？
「特に指摘してほしい観点」を明示する
例:
「早期リターンやガード節の観点からもレビューしてください」
「モジュール分割の余地があれば指摘してください」
「命名の具体性についても厳しく見てください」
「ルールのどの項目を重視するか」を指定する
例:
「命名・ガード節・モジュール分割の3点を重点的にレビューしてください」
「アンチパターンがないかも必ず指摘してください」と明記する
まとめ
AIレビューは「明らかな問題」には強いですが、「より良くするための細かな改善」や「ルールの厳密な適用」には限界があります。
ルールや質問文で「どこを重視するか」「どんな観点で見てほしいか」を明示することで、レビュー精度は大きく向上します。
ルール自体も、具体例やアンチパターン例を増やすことで、AIがより正確に適用しやすくなります。
もし「この観点でのレビュー例が見たい」などご要望があれば、ぜひご指示ください。
暗雲が立ち込めてきましたね。厳しそう。
この回答が本当だとすればルールにはより具体性が必要ということになります。しかし今回実現したいのは具体的なコーディング規約準拠かどうか、というルールを適用させることではなく、可読性を担保するための汎用的なルールを適用することです。
AIフレンドリーになると思って付けているMUSTやSHOULDがかえってルールを曖昧にしてしまっている様子。
MUSTとかSHOULDとか取る、MUST NOT 肯定文 -> 否定文に
のような形でとりあえず改善してみます（根本的な解決にはならなそうですが）。
改善したルール
以下は理解しやすいコードを書くためのベストプラクティスです。
以下のルールに従ってコードレビューしてください。
# Basic Principles
- 可読性や理解容易性を優先する
- 実行フローを追いやすく保つ
# Naming
- 具体的で明確な名前を使用する
- 必要であれば接尾辞や接頭辞で情報を追加する (例: isEnabled, weightKg)
# Layout
- 関連コード間を1行空けて論理ブロックを区切る
- 類似コードを視覚的にそろえ、差分を浮かび上がらせる
# Control Flow
- ループや条件のネストが可読性を損なう場合は、関数抽出・ガード節などで浅く保つ
- 三項演算子は if‑else より 可読性が高くなる場合のみ使用する
# Conditionals
- 調査対象を左辺、比較対象を右辺に置く
```
推奨例: if (length >= 10)
非推奨例: if (10 <= length)
```
- 肯定形を優先する
```
推奨例: if (hasData)
非推奨例: if (!hasData)
```
# Comments
- 「何をするか」より なぜそうするか を説明する。
- 作業途中は TODO:、既知バグは FIXME: プレフィクスを付ける
- 自明なコードや悪実装の言い訳を書かない
# Variables
- スコープを最小化する
- const / readonly を優先しイミュータブルに保つ
- 不要な一時変数を生成しない
# Splitting Large Expressions
- 意味付けの一時変数で式を分割し、可読性を高める
- ド・モルガンの法則を用いて否定条件を簡潔にする
- 複雑な短絡評価で可読性を落とさない
# Modularization
- プロジェクト固有ロジックと汎用ロジックをモジュールで分離する
# Tests
- 目的、前提、期待結果が理解できる
- コストと価値を考慮してテスト範囲を決定する
- 失敗時に原因特定しやすいエラーメッセージを出力する
- テストは小さく独立させ、ヘルパーで重複を排除する
- 100%カバレッジを目的化しない
# Testability
- 関数やクラスは副作用・外部依存を最小化する
- インターフェースをシンプルかつ明確に保つ
- テストの都合で実装の設計・可読性を著しく犠牲にしない
再度レビューさせると...
レビュー結果
# 良い点
- 可読性: 全体的にシンプルで、React Hook Form + Zod によるバリデーションも明確です。
- 命名: 変数名・関数名ともに具体的で分かりやすいです（例: onConfirm, preview, validation）。
- 論理ブロックの区切り: 関連するコード間に空行があり、視認性が高いです。
- イミュータブル: const/useState を適切に使い、状態管理も明確です。
- ガード節: onConfirm 内で preview === null の場合に早期リターンしており、ネストが浅く保たれています。
# 改善点・提案
1. 型の明確化・再利用
Request 型はAPI送信用とコメントされていますが、salary の構造がフォーム値と異なります。型変換の意図が明確ですが、コメントで「API送信用」だけでなく「変換理由」も補足すると親切です。
2. 一時変数の最小化
salaryMatch のスコープは if (preview !== null) 内で十分です。現状問題ありませんが、今後ロジックが増える場合はスコープを意識しましょう。
3. 条件式の肯定形
すでに肯定形 (if (preview !== null)) で書かれており、ルールに準拠しています。
4. コメント
コード全体に「なぜそうするか」のコメントは少なめです。たとえば、salary の正規表現や、onConfirm で reset() する理由など、意図を補足するとより親切です。
5. UI/UX
バリデーションエラーの表示や、確認画面のUIも分かりやすいです。
ただし、alert("送信完了！(コンソールを見てください)"); は開発用の仮実装なので、将来的にはAPI通信やエラーハンドリングに置き換えると良いでしょう。
6. コントロールフロー
else if (preview !== null) の分岐は、if (preview === null) の直後なので、else だけで十分です（冗長な比較を避ける）。
例:
if (preview === null) {
// ...フォーム
} else {
// ...確認画面
}
7. テスト容易性
副作用（alert, console.log, reset）は onConfirm に集約されており、テストしやすい構造です。
# まとめ
全体的にルールに沿った良い実装です。
コメントの追加や、分岐の簡略化（else if → else）でさらに可読性が向上します。
今後API通信やエラーハンドリングを追加する際も、現状の構造を維持すると保守性が高まります。
何か特定の観点でさらに深掘りしたい場合はご指示ください
ガード節: onConfirm 内で preview === null の場合に早期リターンしており、ネストが浅く保たれています
こんなコードはどこにもない。嘘はだめ
すでに肯定形 (if (preview !== null)) で書かれており、ルールに準拠しています。
まあそれはそうなんだけど、この場合は!previewで早期リターンしたほうがよくない？
else if (preview !== null) の分岐は、if (preview === null) の直後なので、else だけで十分です（冗長な比較を避ける）。
ここも!previewで早期リターンが望ましい
まだルールが悪いのかもしれません。AIのことはAIに聞けばいいんですかね。とりあえずChatGPTに投げて改善させてみます。
ChatGPTに修正させたルール
# 📜 Code Review Rules (Improved Version)
---
# Fundamental Principle
- 本ドキュメントは「ガイドライン」と「厳格ルール」の2つに分かれる。
- ガイドラインは判断基準を示すもの、厳格ルールは違反検出を目的とする。
- ガイドラインに従うことを基本とし、より良い選択肢があれば柔軟に判断してよい。
- 厳格ルールは原則すべて遵守し、違反がある場合は修正または明確な理由を説明すること。
---
# 1. ガイドライン (Guidelines)
## 1.1 可読性 (Readability)
- コードは「上から順に読むだけで処理内容を自然にトレースできる」ように書く。
- 無駄な複雑化を避け、シンプルで意図が明確な記述を心がける。
- 実行フローが直感的に追えるように構成する。
## 1.2 命名 (Naming)
- 変数名・関数名は、用途や意味が自然に想像できるものとする。
- 状態を表す変数には接頭辞を付ける（例: `isEnabled`, `hasData`, `canSubmit`）。
- スコープが小さい場合でも、意味を持たない一文字変数 (`x`, `y`) などは避ける。
## 1.3 コードレイアウト (Layout)
- 論理ブロックの間には1行の空白を入れる。
- 類似処理は視覚的に揃え、差分を浮き彫りにする（例：複数の型定義や複数のボタン配置など）。
## 1.4 制御構造 (Control Flow)
- ネストは原則2段以内に収める。
- 条件が増える場合はガード節 (`if (異常系) return;`) を優先して早期リターンする。
- 三項演算子は if-else 文よりも可読性が高い場合にのみ使用する。
## 1.5 コメント (Comments)
- 「何をしているか」ではなく「なぜそうするか」を中心にコメントを書く。
- 作業途中の箇所には `TODO: `、既知の不具合には `FIXME: ` プレフィクスを付与する。
- 自明なコード（加算、代入など）や、悪実装の言い訳コメントは禁止する。
## 1.6 テスト (Tests)
- テストケースは目的、前提、期待結果が明確であること。
- テストは小さく独立させ、ヘルパー関数で重複を排除する。
- テストの目的は品質担保であり、カバレッジ100%を機械的に追求しない。
---
# 2. 厳格ルール (Strict Rules)
以下は違反を自動検出または明示的指摘可能なルール。
## 2.1 命名に関するルール
- 変数名・関数名が2文字以下なら警告する（例: `x`, `do` はNG）。
- 状態を表す変数（boolean型）は `is/has/can` で始める。
## 2.2 ネスト・フローに関するルール
- if/else/forのネストが3段以上ある場合は警告する。
- ガード節を使用できる場面で使用していない場合は指摘する。
- 三項演算子で3つ以上の処理分岐をしている場合は指摘する。
## 2.3 コード量・関数サイズに関するルール
- 関数が50行を超えた場合は警告する。
- 1ファイルに収める関数は原則20個以内とする。
## 2.4 比較・条件式に関するルール
- if文で比較対象の順序を誤っている場合は警告する。
- 推奨例: `if (length >= 10)`
- 非推奨例: `if (10 <= length)`
- 否定条件 (`!isValid`) を連続して二重に使っている場合は警告する。
## 2.5 コメントに関するルール
- `TODO:` または `FIXME:` プレフィクスなしで作業途中・不具合を示している場合は警告する。
- 「自明なこと」をコメントしている場合（例: `// 1を加算する`）は警告する。
## 2.6 テストに関するルール
- テスト名に「何を」「どんな状況で」テストしているかが入っていない場合は警告する。
- テスト内に複雑な前提設定 (`beforeAll`, `setup`) がコメントなしで存在する場合は警告する。
---
# 3. 運用上の注意
- ルールは厳密な「型」ではなく「改善ポイントを示す」ものである。
- 指摘の際は単なる違反指摘だけでなく、なぜ直すべきか、どう直すべきかまでセットで提案する。
- 実際のプロジェクト状況や意図を考慮し、柔軟に運用する。
---
もはやリーダブルコードの思想から若干離れているように感じます。
まあでもせっかくなのでこのルールでレビューさせてみましょう。
レビュー結果
1. ガイドラインに基づくレビュー
1.1 可読性
✅ 良い点:
フォームの構造が明確で、入力フォームと確認画面の2つの状態が分かりやすく分離されています
バリデーションルールが上部で定義されており、入力制約が把握しやすいです
1.2 命名
✅ 良い点:
position, companyName, location など、意味が明確な変数名が使用されています
FormValues, Request など、型名も目的が分かりやすいです
1.3 コードレイアウト
✅ 良い点:
論理ブロック間に適切な空白が入っています
フォームの各フィールドの構造が統一されており、視覚的に整っています
1.4 制御構造
✅ 良い点:
ネストが2段以内に収まっています
条件分岐がシンプルで理解しやすいです
1.5 コメント
⚠️ 改善点:
複雑な処理（特に給与の変換ロジック）にコメントがないため、意図が分かりづらい箇所があります
1.6 テスト
❌ 不足点:
テストコードが見当たりません
2. 厳格ルールに基づくレビュー
2.1 命名に関するルール
✅ すべての変数名が2文字以上で、適切な命名規則に従っています
2.2 ネスト・フローに関するルール
✅ ネストは2段以内に収まっており、適切です
2.3 コード量・関数サイズに関するルール
⚠️ 改善点:
OKJobForm コンポーネントが194行と、50行を超えています
フォームの入力部分と確認画面の表示部分を別コンポーネントに分割することを推奨します
2.4 比較・条件式に関するルール
✅ 条件式は適切に記述されています
2.5 コメントに関するルール
⚠️ 改善点:
給与の変換ロジックなど、複雑な処理にコメントがない箇所があります
改善提案
(以下修正コードの提案)
ダメでした...。
モデルを指定してみる
冒頭で述べた通り、モデルの選択はAutoに設定しています。ですが出力を見る限り何となく毎回GPTっぽい感じがしますね。
そこで、Autoではなく試しにclaude-3.7-sonnetを指定してレビューさせてみました。
結果はどう変わるのでしょうか。
レビュー結果
# 良い点
- 型の定義とバリデーションが明確で堅牢
- Zod + React Hook Formを使用した効果的なフォーム実装
- 条件付きレンダリングによる確認画面フローの実装が直感的
- エラーメッセージの表示が適切
# 改善点
1. 命名に関する問題:
- コンポーネント名が OKJobForm と OK が大文字になっているが、OkJobForm が適切
2. 制御構造の問題:
- onConfirm 関数内の早期リターン後に不要な else があります。これはガード節パターンに従って修正するべきです。
3. サラリー処理のエラーハンドリング不足:
- salaryMatch が null の場合のエラー処理が不十分で、潜在的なバグが発生しやすい
4. コード量の効率化:
- プレビューの表示/非表示でコンポーネント全体を条件分岐しており、可読性低下の原因
5. 可読性とメンテナンス性:
- エラーメッセージが繰り返し表示されるコードがあり、共通コンポーネント化すると良い
# 修正提案
(略。コンポーネント名の修正、ガード節の適用、エラーハンドリング強化、エラーメッセージのコンポーネント化、フォームとプレビューの分離、などのコードを示し、エージェント機能で自動修正してくれた)
# レビューまとめ
OkJobForm コンポーネントに対する主要な改善点とその修正を行いました:
1. 命名の修正: コンポーネント名を正しい形式 OkJobForm に変更しました。
2. ガード節パターンの適用: 早期リターンを使用して不要な else ブロックを排除し、コードの可読性を向上しました。
3. エラーハンドリングの強化: サラリーデータのバリデーションを追加し、不正なフォーマットに対する適切なエラー処理を実装しました。
4. 共通コンポーネントの導入: エラーメッセージ表示用の再利用可能なコンポーネントを作成し、コードの重複を排除しました。
5. コンポーネントの分離: フォームとプレビュー画面を個別のコンポーネントに分割し、各コンポーネントの責務を明確にしました。
これらの修正により、コードの可読性、保守性、エラー処理が向上し、厳格なレビュールールに準拠したコードベースになりました。
え賢い何これステキ。
ちょっと希望が見えてきました。claude-3.7-sonnetを指定すれば最初のルールでもいい感じのレビューをしてくれるかもしれません。試してみます。
レビュー結果
コードレビュー結果:
1. 変数のスコープについて:
- salaryMatch はユニットテスト困難な正規表現処理を含んでいます。これを独立した関数に抽出すべきです。
```
// ... existing code ...
const parseSalary = (salaryStr: string) => {
const match = salaryStr.match(/^(\d+)\s*万円\/(月|年)$/);
return {
amount: Number(match?.[1] || 0),
unit: match?.[2] === "月" ? "month" : "year",
};
};
const onConfirm = () => {
if (preview !== null) {
const requestBody: Request = {
position: preview.position,
companyName: preview.companyName,
location: preview.location,
salary: parseSalary(preview.salary),
description: preview.description,
};
// ... existing code ...
```
2. 条件分岐の簡略化:
- onConfirm 関数の条件分岐で早期リターンが使われていますが、肯定形に変更すると可読性が向上します。
```
// ... existing code ...
const onConfirm = () => {
if (preview === null) return;
const requestBody: Request = {
position: preview.position,
companyName: preview.companyName,
location: preview.location,
salary: parseSalary(preview.salary),
description: preview.description,
};
// ... existing code ...
```
3. コンポーネント分割:
- フォーム表示部分と確認画面が条件分岐で混在しており、可読性が低下しています。各部分を独立したコンポーネントに分割すべきです。
- FormView と PreviewView のような形で分離することで再利用性も向上します。
4. エラーハンドリング:
- API送信時のエラーハンドリングが不足しています。実際の実装では適切なエラー処理を追加すべきです。
5. コメント:
- 正規表現の検証ロジックや、単位変換の理由についての説明コメントがあるとより理解しやすくなります。
6. 型定義:
- Request 型の salary.unit で使用されている union type "month" | "year" は、共通の定数か enum に定義すると再利用性が高まります。
7. 副作用の分離:
- コンソール出力とアラート表示が混在しています。実際のアプリケーションでは、これらのUIロジックとデータ処理ロジックを分離すべきです。
これらの改善により、コードの可読性、テスト容易性、保守性が向上します。
いい感じですね。
別のモデルも試してみます。
gemini-2.5.pro-exp-03-25のレビュー結果
コードレビューを開始します。
全体的に良く書けていると思いますが、いくつかの改善点があります。
# Control Flow
- if (preview === null) と else if (preview !== null) の部分は、単に else で表現できます。preview が null でない場合は、2番目の条件は常に真になります。
# Variables
- onConfirm 関数内の requestBody の構築部分で、salaryMatch が null の場合のフォールバックとして 0 を使用していますが、これは validation の正規表現によって既にバリデーションされているため、基本的には null になることは想定しづらいです。しかし、より安全にするために、salaryMatch が期待通りに値を取得できなかった場合の明確なエラーハンドリングや、型ガードをより厳密にすることも検討できます。現状では、salaryMatch が null の場合に amount が 0、unit が year となってしまいますが、これが意図した動作であるか確認が必要です。
# Layout
- JSX内の各フォーム要素（divで囲まれたラベルと入力フィールドのペア）の間には既にスペースがありますが、論理的な区切りとして一貫性を持たせるために、各要素ブロックの間に1行空行を入れると、さらに視認性が向上するかもしれません。これは主に好みの問題でもありますが、ルールには「関連コード間を1行空けて論理ブロックを区切る」とあります。
# その他
- OKJobForm のコンポーネント名ですが、一般的にReactコンポーネント名はパスカルケース（OkJobForm）で記述されることが多いです。ファイル名が OkJobForm.tsx であるため、コンポーネント名もそれに合わせると一貫性が増します。
これらの点を修正すると、コードがさらに読みやすく、保守しやすくなるでしょう。
具体的な修正案を提示しましょうか？
主に好みの問題でもありますが、ルールには「関連コード間を1行空けて論理ブロックを区切る」とあります。
このあたりが非常にいいと思います。
全体的にいいレビュー結果なのですが、onConfirm内の余計なif-elseの記述は指摘してほしかったです（claude-3.7-sonnetでは指摘されている）。
モデルによって指摘項目のブレがあるのは少しやりづらいです。
最後に、そもそも本当にルールを設定したほうがいいのかどうかを確認するため、ルールなしでレビューさせてみます。
レビュー結果
# 良い点
- Zodを使った型安全なバリデーション実装
- react-hook-formの適切な活用
- エラーメッセージの丁寧な表示
- プレビュー機能の実装
# 改善点
1. エラーハンドリングの強化
- salaryのパース処理でエラーが発生する可能性があります。nullチェックはしていますが、正規表現にマッチしなかった場合の対応が不十分です。
2. コンポーネントの分割
- フォームとプレビューを別コンポーネントに分けると可読性と保守性が向上します。
3. API処理の実装
- 実際のAPIリクエスト処理やエラーハンドリングが未実装です。
- 送信中の状態表示（ローディング）がありません。
4. アクセシビリティの向上
- フォーム要素にaria属性がありません。
5. 型の改善
- Request型のsalary.unitのデフォルト値が明確でない場合があります。
6. UIの一貫性
- ボタンのデザインは統一されていますが、フォーカス状態やホバー状態のスタイルが定義されていません。
いい感じですが、ルールがある場合と比べて若干平凡に思えます。ルールは設定した方がいいですね。
もっと汎用的な、よりリーダブルコードに近いルールにして試したいですが、一旦ここまでにします。
結論と所感
ということで、適切にモデルを選択するとうまいことやってくれました。
ただ、Autoに設定しておいたら勝手に判断してモデルを切り替えてくれるのがCursorのいい点のひとつなので、それが活かせないのは残念です。
具体的な記述方法ではなく、読みやすさや理解しやすさなど認識の根本に関わることをルール化する場合、どうしても文脈や状況によって左右される部分が発生します。
「たしかにルールはそうなんだけど、今回の場合はその限りではない」というような場面が必ずありますし、その状況は多岐にわたるので網羅的にルール化するのは厳しいです。
今回のようなルールを正しく設定するには、そういった状況判断の部分をできる限りAIフレンドリーに記述する必要があると思います。
そのためには「この場合はこう」という条件の羅列ではなく、理想の状態をできる限り正しく宣言する必要がありそうです（対人でも同じかなという気がします）。
参考文献
- Boswell, Dustin・Foucher, Trevor（著）角 征典（翻訳）『リーダブルコード ― より良いコードを書くためのシンプルで実践的なテクニック』
Discussion