関数の出口を1つにする 品質担保の観点で最も重要なコーディングスタンダードと単体テストの関連性に関してのメモ
はじめに
ソフトウェア品質を高める開発者テスト アジャイル時代の実践的・効率的なテストのやり方を読んでいて、「関数の出口を1つにする」という事と単体テストの関連性を少し深堀して理解しておきたいと思った。本記事はその深堀の備忘録です。
背景(前提)
- アーリーリターン (early return) を多用すると、関数内に
return
が散乱しやすい- 処理フローが分散し、保守や拡張の際に「どこで終了するか」を追いかけづらい
- エラー処理やクリーンアップのコードを書き忘れてしまうリスクが高まる
関数の出口を1つにする」とは?
-
「出口を1つにする」 とは、基本的に関数の終了時点を1箇所(または少数の箇所)に集約するコーディングスタイル
- 不要に早期リターンを乱発しない
- 途中の処理がどうであれ、最後に1つの
return
で結果を返す - 処理フローが見通しやすく、バグや漏れを防ぎやすい
単体テストと「出口は1つにすべき」の関係性
-
単体テストは「in/out のみをチェックする」のがシンプルで保守性が高い
- 関数内部の「どんなアルゴリズム・分岐を通ったか」ではなく、最終的な戻り値や外部への副作用だけを確認すればよい
- 実装が変わっても、最終結果(out)が変わらなければテストは壊れず、保守しやすい
-
出口が1つなら最終結果が常に同じ場所から返される
- 「引数Aを与えたときに最終的な戻り値や保存先はどうなるか?」をシンプルにテストしやすい
- 関数内部の分岐が変わっても、出口が変わらなければテストが壊れにくい
上記の2点に関して具体的に見ていくと、例えばvitestでは以下のような確認(toBeCalledWith
など)をすることで処理内容の確認などもできるが、これはin/outのみを確認するシンプルなテストにするという意味では望ましくない。最終的な戻り値のみを確認する方が途中の処理が多少変わってもテスト自体は変更不要で壊れにくくなる。
以下のコードでいえば、expect(success).toBeCalledWith(...)
やexpect(failure).toBeCalledWith(...)
の部分のみを確認するテストにすべき。
※モックにするか否かは今回は議論の対象にしない。
// https://vitest.dev/guide/mocking.html#example-2
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
import { Client } from 'pg'
import { failure, success } from './handlers.js'
// get todos
export async function getTodos(event, context) {
const client = new Client({
// ...clientOptions
})
await client.connect()
try {
const result = await client.query('SELECT * FROM todos;')
client.end()
return success({
message: `${result.rowCount} item(s) returned`,
data: result.rows,
status: true,
})
}
catch (e) {
console.error(e.stack)
client.end()
return failure({ message: e, status: false })
}
}
vi.mock('pg', () => {
const Client = vi.fn()
Client.prototype.connect = vi.fn()
Client.prototype.query = vi.fn()
Client.prototype.end = vi.fn()
return { Client }
})
vi.mock('./handlers.js', () => {
return {
success: vi.fn(),
failure: vi.fn(),
}
})
describe('get a list of todo items', () => {
let client
beforeEach(() => {
client = new Client()
})
afterEach(() => {
vi.clearAllMocks()
})
it('should return items successfully', async () => {
client.query.mockResolvedValueOnce({ rows: [], rowCount: 0 })
await getTodos()
expect(client.connect).toBeCalledTimes(1)
expect(client.query).toBeCalledWith('SELECT * FROM todos;')
expect(client.end).toBeCalledTimes(1)
expect(success).toBeCalledWith({
message: '0 item(s) returned',
data: [],
status: true,
})
})
it('should throw an error', async () => {
const mError = new Error('Unable to retrieve rows')
client.query.mockRejectedValueOnce(mError)
await getTodos()
expect(client.connect).toBeCalledTimes(1)
expect(client.query).toBeCalledWith('SELECT * FROM todos;')
expect(client.end).toBeCalledTimes(1)
expect(failure).toBeCalledWith({ message: mError, status: false })
})
})
- 出口が多いとテストのパターンが増え、複雑になる
- 途中で
return
がいくつも存在すると、その分テストすべきパスも増える - 「ここで return される場合」「別の分岐で return される場合」など、テスト漏れのリスクが高い
- 途中で
上記の点について具体的に見ていくと、例えば以下のような関数をテストしようとすると、どこのリターンをテストしているのか?がぱっと判別不可能であり、テスト漏れのリスクが高まる。
// 悪い例: アーリーリターンが乱立し、途中で何度も抜ける
async function processOrders(orders) {
// 1. 前処理
let result = [];
let dbConnection = null;
try {
dbConnection = await getDbConnection(); // DB接続
if (!orders || orders.length === 0) {
console.log("No orders to process");
await dbConnection.close(); // リソース解放
return result; // アーリーリターン①
}
// 2. バリデーション(一部だけを検証して早期リターン)
if (orders.some(o => !o.id)) {
console.log("Found invalid order (missing id)");
await dbConnection.close(); // リソース解放
return []; // アーリーリターン②
}
// 3. 処理
for (const order of orders) {
if (order.status === 'CANCELLED') {
console.log("Skip cancelled order");
continue;
}
if (order.amount < 0) {
console.log("Skip negative amount");
continue;
}
// 実際に注文を処理
const res = await processSingleOrder(order, dbConnection);
result.push(res);
}
// 4. 後処理なしに return
return result; // アーリーリターン③
} catch (err) {
console.error("Error in processOrders:", err);
return []; // 早期リターン④
} finally {
if (dbConnection) {
await dbConnection.close(); // ここにもリソース解放
}
}
}
アーリーリターンが多すぎる実装のデメリットのまとめ
-
保守性の低下
- 途中で抜けるパスが多いと、後で修正や要件変更があった際、「各パスに共通処理を挿入し忘れる」といったバグを招きがち
-
可読性の低下
- 「どの条件で return が走るか」を追うだけで読む人の負担が増える
-
テスト漏れのリスク
- 各パスを個別にテストしないといけない
- 大きな関数になるほど出口パターンが煩雑になり、網羅しづらい
具体的な実装・テストのイメージ
-
悪い例
- リソース解放やエラー処理などが、何度も登場する途中の
return
でバラバラに書かれている - 例:
if (orders == null) return; if (err) return; ... さらに try/catch/finally で return; ...
といった構造
- リソース解放やエラー処理などが、何度も登場する途中の
-
望ましい例
- 「入り口でパラメータチェック → メイン処理 → 出口でまとめて
return
」 という構成 - エラーは例外(throw)で一括管理し、catch ブロックで最終的にまとめて処理
- 正常系でも最終的に1つの
return
で結果を返す
- 「入り口でパラメータチェック → メイン処理 → 出口でまとめて
※短い分岐であればアーリーリターンを少し使っても可読性が損なわれない場合もある
分岐が増えてきたら、switch
や if-else
などで一本化し、出口をまとめるほうが保守しやすい
例えば、以下のようなVue.jsのメソッド(オプションAPIの記法)の実装は、アーリーリターンを用いているが、関数の行数も少なくシンプルなので問題にならない。
changeTab() {
if (this.tab === 'a') {
this.fileLabel = 'aのEXCELファイル';
return;
}
if (this.tab === 'b') {
this.fileLabel = 'bのEXCELファイル';
return;
}
if (this.tab === 'c') {
this.fileLabel = 'cのEXCELファイル';
return;
}
this.fileLabel = 'dのEXCELファイル';
}
また、単体テストを書く場合にも、in/outの原則を適用してテストを書くこともできる(Vue.jsではthis
を使用してdata
を更新したりするが、つまりはそのdata
の最終状態を確認するテストにすれば、in/outのみを確認するシンプルなテストと同じように単体テストを書くことができる)。
// pseudo code (Jest ベースの例)
describe('changeTab method', () => {
let wrapper;
beforeEach(() => {
wrapper = shallowMount(MyComponent, {
data: () => ({
tab: '',
fileLabel: '',
}),
});
});
test('tab = a の場合', () => {
wrapper.setData({ tab: 'a' });
wrapper.vm.changeTab();
expect(wrapper.vm.fileLabel).toBe('aのEXCELファイル');
});
test('tab = b の場合', () => {
wrapper.setData({ tab: 'b' });
wrapper.vm.changeTab();
expect(wrapper.vm.fileLabel).toBe('bのEXCELファイル');
});
test('tab = c の場合', () => {
...
});
test('tab = その他 の場合', () => {
...
});
});
まとめとして
- 出口を1つor最小限に抑える ことで、関数のフローが明確になり、バグ・漏れ・テスト漏れを防ぎやすい
- 出口を1つにのコーディングスタンダードは、単体テストを in/out ベースで書く方針 との相性が良く、実装のリファクタリング後もテストが壊れにくい
- 小さい関数・単純な分岐なら多少の早期リターンも問題ないが、複雑化しそうなら出口を1つにまとめる 方が品質向上に繋がる
Discussion