16
3

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?

@Nao52(Naoki Saito)in株式会社シンシア

設計本の学びがコードレビューで活きた話｜フラグ引数・null戻り値・型の網羅性

16
Posted at

■ この記事はこんな人におすすめ

  • TypeScript / Next.js を使って実務開発をしている人
  • コードレビューで設計的な指摘をしたい・受けたい人
  • 「良いコード／悪いコードで学ぶ設計入門」を読んで実務に活かしたい人
  • null を返す関数や boolean フラグ引数に違和感を感じたことがある人

■ この記事で得られること

  • フラグ引数・null 戻り値・型の網羅性という3つの設計問題を具体的に理解できます
  • 実務のコードレビューで設計の観点から指摘・改善できるようになります
  • 書籍の学びを実コードに落とし込む思考プロセスが身につきます

■ 参考書籍

■ 関連記事（上記での学びを記事にしています）

1. 結論

設計の書籍で学んだ原則は、実際のコードレビューの場で「なぜこのコードが良くないのか」を言語化する力を与えてくれます。

今回のレビューで指摘したコードには、以下の 3つの設計上の問題 が含まれていました。

  1. 型の網羅性が担保されていない（ロールが増えても静的に検知できない）
  2. フラグ引数を使っている（関数が複数の責務を持っている）
  3. null 戻り値の意図がわかりにくい（成功・失敗が型で表現されていない）

これらはどれも『良いコード／悪いコードで学ぶ設計入門』で取り上げられている概念と直結しており、書籍の言葉を借りながらレビューコメントを書くことができました。

2. 具体的にやること

■ ① 指摘対象のコードを読む

まず、レビューで問題になったコードを確認します。

export async function requireTenantAccess(
  tenantId: number,
  options: { designerOrAdminOnly?: boolean } = {}
): Promise<NextResponse<ApiErrorResponse> | null> {
  const session = await getSession();
  if (!session) return errorResponse('Unauthorized', { status: 401 });

  if (options.designerOrAdminOnly && session.user.role === UserRole.CLIENT) {
    return errorResponse('Forbidden', { status: 403 });
  }

  if (session.user.role === UserRole.ADMIN) return null;

  const membership = await prisma.tenantMember.findUnique({
    where: { userId_tenantId: { userId: session.user.id, tenantId } },
  });

  if (!membership) return errorResponse('Forbidden', { status: 403 });

  return null;
}

パッと見は動いていますが、設計の観点から3つの問題が潜んでいます。

■ ② 問題①：ロールが増えても静的に検知できない

ロールの分岐が if 文で書かれており、将来 SUPER_ADMIN や OWNER などのロールが追加された際にコンパイルエラーで検知できません。

// 問題：SUPER_ADMIN ロールの扱いが if 文に記載されていない
// → 新ロール追加時に考慮漏れが起きてもコンパイルは通ってしまう
if (options.designerOrAdminOnly && session.user.role === UserRole.CLIENT) { ... }
if (session.user.role === UserRole.ADMIN) return null;

Record<Role, ...> を使うことで、全ロールに対する定義が強制され、追加忘れをコンパイル時に検知できます。

const TENANT_ACCESS_CONFIG: Record<Role, { allowed: boolean; skipMembershipCheck: boolean }> = {
  ADMIN:    { allowed: true,  skipMembershipCheck: true  },
  DESIGNER: { allowed: true,  skipMembershipCheck: false },
  CLIENT:   { allowed: true,  skipMembershipCheck: false },
};
// → 新ロールを Role 型に追加すると、ここでコンパイルエラーになる

■ ③ 問題②：フラグ引数を使っている

options: { designerOrAdminOnly?: boolean } はフラグ引数です。

フラグ引数は「この関数は条件によって異なる振る舞いをする」というサインであり、単一責任の原則に反します。

// NG：フラグによって挙動が変わる
await requireTenantAccess(tenantId, { designerOrAdminOnly: true });
await requireTenantAccess(tenantId);

今回のリファクタでは、ロールごとの設定を TENANT_ACCESS_CONFIG として外部に切り出すことで、関数自体はアクセス制御の実行だけに集中できるようになりました。

// OK：関数はアクセス判定だけに集中
await requireTenantAccess(tenantId);

■ ④ 問題③：null 戻り値の意図がわかりにくい

元のコードでは「エラーがなければ null を返す」という設計になっています。

// 変更前の呼び出し側
const authError = await requireTenantAccess(tenantId);
if (authError) return authError;

null は「成功」を意味しますが、型だけを見ても意図が読み取れません。「null が返ってきたらどういう状態なの？」と読み手を迷わせます。

戻り値を { ok: true } | { ok: false; response: ... } という Result 型 で表現することで、意図が明確になります。

// OK：成功・失敗が型で表現されている
type AccessResult =
  | { ok: true }
  | { ok: false; response: NextResponse<ApiErrorResponse> };

// 呼び出し側も意図が明確
const access = await requireTenantAccess(tenantId);
if (!access.ok) return access.response;

3. リファクタ後のコード全体

type AccessResult =
  | { ok: true }
  | { ok: false; response: NextResponse<ApiErrorResponse> };

const TENANT_ACCESS_CONFIG: Record<Role, { allowed: boolean; skipMembershipCheck: boolean }> = {
  ADMIN:    { allowed: true,  skipMembershipCheck: true  },
  DESIGNER: { allowed: true,  skipMembershipCheck: false },
  CLIENT:   { allowed: true,  skipMembershipCheck: false },
};

export async function requireTenantAccess(
  tenantId: number,
): Promise<AccessResult> {
  const session = await getSession();
  if (!session) return { ok: false, response: errorResponse('Unauthorized', { status: 401 }) };

  const { allowed, skipMembershipCheck } = TENANT_ACCESS_CONFIG[session.user.role];

  if (!allowed) return { ok: false, response: errorResponse('Forbidden', { status: 403 }) };

  if (skipMembershipCheck) return { ok: true };

  const membership = await prisma.TenantMember.findUnique({
    where: { userId_tenantId: { userId: session.user.id, tenantId } },
  });

  if (!membership) return { ok: false, response: errorResponse('Forbidden', { status: 403 }) };

  return { ok: true };
}

まとめ

問題 原因 解決策
ロール追加時に検知できない if 文での分岐 Record<Role, ...> で網羅性を強制
フラグ引数 関数が複数の責務を持つ ロールごとの設定オブジェクトに切り出す
null 戻り値が不明瞭 成功状態が型で表現されていない Result 型 { ok: true/false } で明示

結局何をすればいいの❓

  • 今すぐ： 自分のコードで null を返している関数を探し、明示的な型に置き換えられないか検討する
  • レビュー時： フラグ引数を見かけたら「関数が複数の責務を持っていないか」を確認する
  • 習慣として： Union 型や Record を活用して、型レベルで網羅性を担保する設計を意識する

設計本の学びは、コードレビューで「なんとなく嫌」を「言語化できる指摘」に変えてくれます。ぜひ書籍と実務をセットで学んでみてください。

株式会社シンシア

株式会社xincereでは、実務未経験のエンジニアの方や学生エンジニアインターンを採用し一緒に働いています。
※ シンシアにおける働き方の様子はこちら

シンシアでは、年間100人程度の実務未経験の方が応募し技術面接を受けます。
その経験を通し、実務未経験者の方にぜひ身につけて欲しい技術力（文法）をここでは紹介していきます。

16
3
0

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
Sign upLogin
16
3

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?