例外処理コードレビュー集
最近、よく他の方が書いたコードをレビューしているのですが、なぜか同じようなレビューコメントをするケースが多いので自分の中で整理のためにまとめてみました。TypeScriptです。
1.catchしたエラーをそのままthrowするやつ
元コード
catchした例外をそのままthrowするだけのコード。catch句でなにか重要な処理をしているのかと期待して思って読み進めると結局何もやってなかった。インデントが増えただけ、みたいな状況。
try {
: // なにか処理
} catch (error) {
// catchしたエラーをそのままthrowするだけ
throw error;
}
変更案
無意味なtry~catchは脊髄反射的に削除したい。すべての関数でtry~catchする必要はない。
: // なにか処理
2.catchしたエラーを捨てて新しい例外をthrowするやつ
元コード
catchしたerrorオブジェクトを捨ててわざわざ新しい例外をthrowしているコード。元のerrorオブジェクトに含まれるmessageやstackなどの重要な情報を捨ててしまったことにより、これ以上深いデバッグが難しくなる。
try {
: // なにか処理
} catch (error) {
// catchしたエラーに含まれる重要な情報を捨てている。
throw new Error('新しい例外が起きたよ！');
}
変更案1
例外がthrowされればよいのであれば、余計なtry~catchを削除すればいい。
: // なにか処理
変更案2
特定の例外クラスをthrowすることに意味があるなら、catchしたerrorを引数で受ける例外クラスを実装しても良いかもしれない。フレームワーク側がこの手の例外クラスを用意してくれるケースは結構ありそう。
try {
: // なにか処理
} catch (error) {
// catchしたerrorを引き回している。
throw new CustomError(error);
}
class CustomError extends Error {
public constructor(original: any) {
super(original.message);
}
}
3.下位の関数でcatchしてログ出力だけしているやつ
元コード
下位の関数のcatch句でログ出力だけしてそのままthrowしているコード。このタイミングでログ出力することに意味があるなら否定するものではないが、こんなコードがプログラムのあちこちに散らばっていたらログの出力漏れが心配だし、ログのフォーマットを変更しようと思った時に修正箇所が増える。
function child1() {
try {
: // なにか処理
} catch (error) {
// 下位の関数でログを出力するためだけのcatchは必要だろうか。
// より上位階層にエラー処理を集約できないか検討すべき。
console.error(error);
throw error;
}
}
function parent() {
child1();
child2();
child3();
}
変更案
例外処理を上位階層に集約する。例外処理は関数単位で考えるのではなく、呼び出し階層全体で考えて欲しい。
function child1() {
: // なにか処理
}
function parent() {
try {
child1();
child2();
child3();
} catch (error) {
// エラー処理を上位階層に集約する
console.error(error); // っていうか、上位にthrowするならログ出力必要？
throw error;
}
}
4.エラーを握りつぶすやつ
元コード
catchしたエラーを闇に葬り去る最高クラスの凶悪なコード。例外が発生したことに誰も気付けない。
try {
: // なにか処理
} catch (error) {
// 何もなかったことにする(^o^)v
}
変更案
発生した例外は漏れなく上位に伝えて欲しい。バグがあるなら目を背けず直して欲しい。
: // なにか処理
5.正常系でも異常系でも同じことをするやつ
元コード
例えば、何が起きてもログ出力する仕様で、素直に正常系、異常系それぞれでログ出力しているコード。これだと正常系で早期returnするようリファクタするとログが出力されなくなってしまいそう。
try {
: // なにか処理
console.log("成功");
} catch (error) {
console.log("失敗");
throw error;
}
変更案
必ず何かを実行したいならfinallyを使って欲しい。これなら必ずログが出力される。
let message = "失敗";
try {
: // なにか処理
message = "成功";
} finally {
console.log(message);
}
6.nullを返すことでエラーを表現するやつ
元コード
child()で起きた異常を戻り値のnullで表現しているコード。呼び出し元（parent()）で戻り値をチェックしてnullなら例外をthrowしてそれをcatchしている。これだとstackに記録されている例外の発生場所と異常を検知した場所から離れているので調査に一手間かかる。戻り値のnullになにか意味があるのかと思わせて読み手を翻弄してくれた。あと、戻り値のチェックを忘れていないか心配になる。
function child(): number | null {
if (なにかを判定) { // <- 問題を検知した場所
return null;
}
return 123;
}
function parent() {
try {
const result = child();
if (result == null) {
throw new Error('エラー'); // <- stackに記録される場所（問題を検知した場所から遠い）
}
} catch (error) {
: // 例外処理
}
}
変更案
異常が起きたらその場で例外をthrowする。それを必要な場所でcatchする。これにより、問題を検知した場所とstackに記録される場所が近くなりデバッグしやすい。
function child(): number | null {
if (なにかを判定) { // <- 問題を検知した場所
throw new Error('エラー'); // <- stackに記録される場所（問題を検知した場所と近い）
}
return 123;
}
function parent() {
try {
const result = child();
} catch (error) {
: // 例外処理
}
}
7.場所決め打ちでtry~catchするやつ
元コード
たくさん処理がある中で決め打ちでtry~catchをしているコード。それ以外の場所で例外が出る可能性を考慮しなくて大丈夫？try~catch外でどんな例外が出るか読み手に確認させるつもり？
: // なにか処理
: // なにか処理
try {
: // なにか処理
} catch(error) {
: // 例外処理
}
: // なにか処理
: // なにか処理
変更案
どこで例外が発生しても同じ処理をするならその部分はまとめて囲っておいた方がよい。例外の発生箇所はstackを見れば特定できる。もちろん、特定の場所で例外処理を変えたいなら元コードのようにピンポイントでtry~catchするのはありだが、「ここで例外が起きるはず」という決め打ちをして良いのかはよく考えてほしい。
try {
: // なにか処理
: // なにか処理
: // なにか処理
: // なにか処理
: // なにか処理
} catch(error) {
: // 同じ例外処理
}
8.catch内でtry~catchするやつ
元コード
catch内でさらにtry~catchをしているコード。なんか複雑で読みたくない。
try {
: // なにか処理
} catch(error) {
try {
: // なにか処理
} catch(error) {
: // 例外処理中の例外処理
}
: // 例外処理
}
変更案
正常系を中断して例外処理をしているのであれば、さっさと諦めて上位にエラーを伝えることに専念して欲しい。どこで例外が起きてもstackとmessageを見れば判別がつく。もしかすると、他の実装バグの尻拭いをしていないか考え直した方がいいかもしれない。
try {
: // なにか処理
} catch(error) {
: // 例外処理
}
まとめ
最後に、自分だったらこう考える、というのをまとめてみます。
- まずは正常系だけ書く
- いきなりtry~から書き始めない
- 必要に迫られない限り例外処理はしない
- リソースの解放や利用者向けのメッセージ調整、エラー発生時のレスポンスの調整など、個別に必要なケースに限り例外処理する
- なるべく上位で例外処理する
- WebバックエンドならControllerより上位の共通層(NestJSならException filterあたり）
- batchならエントリーポイントの階層（index.tsあたり）
- フロントエンドならコンポーネントレベル
- 例外が起きたことは漏れなく上位に伝える（ただし、伝え方や詳細度は利用者に合わせて要調整）
- WebバックエンドならWebリクエストのレスポンスでエラーを表現
- batchなら関数の終了コードや戻り値
- フロントエンドならユーザー向けのメッセージ＋開発担当者向けのエラーを分ける
- 例外が起きることを恐れない。検知する仕組みさえあればどうにでもなる。
try~catchで囲うだけなのに、集めてみたら意外とたくさんありました。
参考情報
JavaScript/TypeScriptの例外周りの情報
Discussion