はじめに
筆者はSaaSアプリケーションのPdMとしてより高い品質のプロダクトを開発すべく活動しています。この一年間で78,000行のコードをレビューし、1,700件のコメントを残してきました。このレビュー経験から見えてきたコードレビューの観点をまとめていきます。
コードレビューの目的
SaaSアプリケーションは常に変化し続ける「テセウスの船」のような存在です。あらゆる箇所に変更が加わることを想定し、合理的で可読性や保守性の高いコード以外を許容してはなりません。この記事ではコードレビューの観点を以下の3つに分類し、それぞれを具体例とともに解説します。
- 合理性
- 可読性
- 保守性
これら3つの観点は互いに関連し合っていますが、本記事では思考を整理するために分類して解説します。
(仕様やセキュリティ面などの、議論の余地なく修正する必要のある内容は省略します)
合理性
リソースを無駄にしていないか
N+1クエリ、多重のループ処理などを避けることでリソースを節約できます。
N+1クエリ問題の回避
// 悪い例: N+1クエリ問題
const getPostsWithAuthors = async () => {
const posts = await db.query('SELECT * FROM posts');
for (const post of posts) {
const author = await db.query(
'SELECT * FROM users WHERE id = ?',
[post.author_id]
);
post.author = author[0];
}
return posts;
};
// 良い例: JOINで一度に取得
const getPostsWithAuthors = async () => {
return await db.query(`
SELECT p.*, u.name as author_name, u.email as author_email
FROM posts p
JOIN users u ON p.author_id = u.id
`);
};
二重ループの最適化
// 悪い例: 二重ループでO(m * n)の処理
const getUsersWithPosts = (users: User[], posts: Post[]) => {
return users.map(user => ({
...user,
posts: posts.filter(post => post.userId === user.id)
}));
};
// 良い例: Mapを使ってO(m + n)に最適化
const getUsersWithPosts = (users: User[], posts: Post[]) => {
const postsByUserId = new Map<string, Post[]>();
posts.forEach(post => {
const posts = postsByUserId.get(post.userId);
if (!posts) {
postsByUserId.set(post.userId, [post]);
} else {
posts.push(post);
}
});
return users.map(user => ({
...user,
posts: postsByUserId.get(user.id) || []
}));
};
車輪を再発明していないか
よくある処理については自分で実装するより既存のライブラリやフレームワークのコードを利用した方が品質を高められる場合が多いです。また実装の時間効率も向上します。
// 悪い例: 自前でUUID生成
const generateId = () => {
return Math.random().toString(36).substr(2, 9) +
Date.now().toString(36);
};
// 良い例: ライブラリを使用
import { v4 as uuidv4 } from 'uuid';
const generateId = () => uuidv4();
言語機能を適切に使っているか
言語が提供する便利な構文を適切に使用することで可読性を向上させることができます。
switch文の活用
// 悪い例: ネストした三項演算子
const getMessage = (status: string) => {
return status === 'active' ? 'アクティブ' :
status === 'inactive' ? '非アクティブ' : '不明';
};
// 良い例: switch文を使用
const getMessage = (status: string) => {
switch (status) {
case 'active': return 'アクティブ';
case 'inactive': return '非アクティブ';
default: return '不明';
}
};
Enumの活用
enum UserStatus {
ACTIVE = 'active',
INACTIVE = 'inactive',
PENDING = 'pending',
}
const getMessage = (status: UserStatus) => {
switch (status) {
case UserStatus.ACTIVE: return 'アクティブ';
case UserStatus.INACTIVE: return '非アクティブ';
case UserStatus.PENDING: return '保留中';
}
};
エスケープハッチを安易に利用していないか
言語やフレームワークの制約を回避する手段を安易に利用してしまうと、コードの可読性や保守性が低下します。
TypeScriptのany型
// 悪い例: any型を使用
const processData = (data: any) => {
return data.items.map((item: any) => item.name);
};
// 良い例: 型を明示的に定義
interface Item {
id: number;
name: string;
}
const processData = (data: { items: Item[] }) => {
return data.items.map(item => item.name);
};
Rustのunwrap()の使用
// 悪い例: unwrap()を使用
let data = get_data().unwrap();
// 良い例: Result型を使ってエラーハンドリング
let data = match get_data() {
Ok(data) => data,
Err(e) => {
eprintln!("Error: {}", e);
return;
}
};
関数の純粋性が保たれているか
関数は副作用を持たない純粋な関数にすることを意識すると予測可能で理解しやすいコードになります。特にmap()やfilter()などの関数型プログラミングの関数を使うときは注意が必要です。
// 悪い例: 副作用のある関数
let count = 0;
const processItems = (items: Item[]) => {
return items.map(item => {
count++; // 外部状態を変更（副作用）
return { ...item, processed: true };
});
};
// 良い例: 純粋な関数
const processItems = (items: Item[]) => {
return items.map(item => ({ ...item, processed: true }));
};
関数の契約を守っているか
関数の契約(何を受け取ってどんな処理をするのか)を守ることを意識して書くことも重要です。
// 悪い例: 関数名から想像される処理を行わない
function getUser(id: string): User {
const user = users.find(u => u.id === id);
if (!user) {
// ユーザーを取得するだけのはずが、新しいユーザーを作成している
const newUser = { id, name: 'Unknown', email: '' };
users.push(newUser);
return newUser;
}
return user;
}
// 良い例: 関数名通りの処理を行う
function getUser(id: string): User | null {
return users.find(u => u.id === id) || null;
}
// 必要であれば別の関数として定義
function createUserIfNotExists(id: string): User {
const user = getUser(id);
if (!user) {
const newUser = { id, name: 'Unknown', email: '' };
users.push(newUser);
return newUser;
}
return user;
}
可読性
内容を適切に表現する名前になっているか
変数や関数の名前は、その内容を正確に表現する必要があります。不適切な名前をつけてしまうとそれを利用したコードも意味不明になってしまいます。また適切な名前をつけておけばコードを読むときに内容を意識せずに済むため、認知負荷が軽減されます。
// 悪い例: 微妙に意味がずれている名前
const userList = getActiveUsers();
const maxPrice = Math.min(...prices);
const isValid = checkPassword(password);
// 良い例: 内容を適切に表現する名前
const activeUserList = getActiveUsers();
const minPrice = Math.min(...prices);
const isStrong = checkPassword(password);
理解しやすいインターフェースになっているか
言語機能やライブラリの仕様から自然に想像できるインターフェースを設計します。直観に反するインターフェースだとコードを理解するのが難しくなります。
例外処理の適切な使用
// 悪い例: 分かりにくいインターフェース
const findUser = (id: string) => {
const user = users.find(u => u.id === id);
return user ? user : null;
};
// 良い例: 理解しやすいインターフェース
const findUser = (id: string) => {
const user = users.find(u => u.id === id);
if (!user) {
throw new Error(`User not found: ${id}`);
}
return user;
};
自然なイベントハンドラ名
// 悪い例: 分かりにくいインターフェース
const Button = ({ click, text }) => {
return <button onClick={click}>{text}</button>;
};
// 良い例: 理解しやすいインターフェース
const Button = ({ onClick, children }) => {
return <button onClick={onClick}>{children}</button>;
};
命名規則が統一されているか
一貫した命名規則により、コードの可読性が向上します。言語によってキャメルケース/スネークケースなどの命名規則があるのでそれに従うことが重要です。
// 悪い例: 命名規則が統一されていない
const user_name = "田中太郎";
const userAge = 25;
const UserEmail = "tanaka@example.com";
// 良い例: キャメルケースで統一
const userName = "田中太郎";
const userAge = 25;
const userEmail = "tanaka@example.com";
多値論理を使っていないか
人間の脳は多値論理(true/false/null/undefinedなど)を扱うのが苦手なため、可能な限りtrue/falseの二値論理にすると理解しやすくなります。
// 悪い例: 多値論理
interface User {
status: 'active' | 'inactive' | 'pending' | null;
}
const canAccess = (user: User) => {
if (user.status === 'active') return true;
if (user.status === 'inactive') return false;
if (user.status === 'pending') return false;
return false;
};
// 良い例: 二値論理
interface User {
isActive: boolean;
isPending: boolean;
}
const canAccess = (user: User) => {
return user.isActive && !user.isPending;
};
保守性
重複したコードはないか
コードが重複していると変更時に複数箇所を修正する必要があるため、保守性が低下します。
// 悪い例: 重複したコード
const validateEmail = (email: string) => {
if (!email.includes('@')) return false;
if (email.length < 5) return false;
return true;
};
const validateUsername = (username: string) => {
if (!username.includes('@')) return false;
if (username.length < 5) return false;
return true;
};
// 良い例: 共通化
const validateInput = (input: string, hasAtSymbol: boolean = false) => {
if (hasAtSymbol && !input.includes('@')) return false;
if (input.length < 5) return false;
return true;
};
const validateEmail = (email: string) => validateInput(email, true);
const validateUsername = (username: string) => validateInput(username);
デッドコードが残っていないか
使用されていないコードは積極的に削除します。バージョン管理システムによって過去のコードはいつでも復元可能なので、不要なコードを残す必要はありません。
// 悪い例: デッドコードが残っている
const calculateTotal = (items: Item[]) => {
// 古い実装（使われていない）
// let total = 0;
// for (let i = 0; i < items.length; i++) {
// total += items[i].price;
// }
return items.reduce((sum, item) => sum + item.price, 0);
};
// 良い例: 不要なコードを削除
const calculateTotal = (items: Item[]) => {
return items.reduce((sum, item) => sum + item.price, 0);
};
コードから自然と理解できる内容をコメントに書いていないか
コードから自然に理解できる内容をコメントに書く必要はありません。コードの修正とコメントの修正がずれると誤解を招く原因になります。
// 悪い例: コードから明らかな内容をコメントに書いている
const users = [];
// ユーザーを配列に追加する
users.push(newUser);
// ユーザー数を取得する
const userCount = users.length;
// 良い例: 必要な場合のみコメントを書く
const users = [];
users.push(newUser);
const userCount = users.length;
// 複雑な処理にのみコメントを追加
// 外部APIの制限により、一度に取得できるのは100件まで
const batchSize = 100;
マジックナンバーが使われていないか
特別な意味のある値は変数として定義して参照するようにします。直接記述されているとその値が何を意味するのか分かりにくくなります。
// 悪い例: マジックナンバーを使用
const validatePassword = (password: string) => {
if (password.length < 8) {
return false;
}
return true;
};
// 良い例: 定数として定義
const minimumPasswordLength = 8;
const validatePassword = (password: string) => {
if (password.length < minimumPasswordLength) {
return false;
}
return true;
};
リテラル値が手書きされていないか
リテラル値を手書きするとタイポや変更漏れの原因になります。マジックナンバーと同様に変数として定義して参照します。
// 悪い例: 同じリテラル値が重複している
const createUser = (name: string) => {
return { name, status: 'active', role: 'user' };
};
const updateUserStatus = (id: string) => {
return db.users.update({ where: { id }, data: { status: 'active' } });
};
// 良い例: 定数として定義
enum UserStatus {
ACTIVE = 'active',
INACTIVE = 'inactive'
};
const createUser = (name: string) => {
return { name, status: UserStatus.ACTIVE, role: 'user' };
};
const updateUserStatus = (id: string) => {
return db.users.update({
where: { id },
data: { status: UserStatus.ACTIVE }
});
};
質の高いコードがもたらす効果
大量のコードを扱う開発では、脳のRAMをいかに効率的に運用するかが重要な要素となります。変数名が適切であったり、理解しやすいインターフェースになっているコードは内容を意識せず扱うことができます。つまり、情報を圧縮して脳のRAMの容量を節約することができます。逆に品質の低いコードは脳のRAMを圧迫し、開発効率を低下させます。
まとめ
コードレビューは合理的で可読性や保守性の高いコードベースを維持するために重要な活動です。またそうしたコードが維持されることは、将来的な保守だけでなく開発効率の向上にも寄与します。
We Are Hiring!
VALUESでは「組織の提案力と生産性を最大化する提案ナレッジシェアクラウド」Pitchcraftなどの開発メンバーとしてエンジニアを積極採用中です。