ソースコードのチェックポイント
自身がソースを書く時もそうだが、人のソースをチェックするときにも必要になる観点を備忘録としてまとめておく。
マジックナンバーを使っていないか
if (count > 10) {
return "err_msg";
}
何が悪いのか
上記のソースでは「10」「err_msg」が何を表すものであるのか直感的にわかりません。
定数やenumを使うなどしてわかりやすくしましょう。
修正
final int MAX_USER_COUNT = 10;
final String ERR_MSG_OVER_MAX_USER_COUNT = "err_msg";
...(中略)...
if (count > MAX_USER_COUNT) {
return ERR_MSG_OVER_MAX_USER_COUNT;
}
どう良くなったか
if文の条件を読んだときに、ユーザーの上限を超えた時にエラーメッセージを返すことがわかるようになりました。
また、どういったエラー内容が返るのかイメージも付きやすくなりました。
フォーマットがかかっているか
private String manpukuStatus(int amount) {
if (amount > STATUS_FULL) {return "満腹";
} else if (amount < STATUS_MAX_NORMAL&&10 < STATUS_MIN_NORMAL) {return "腹八分目";
}else{return "おなかが減っている";}}
何が悪いのか
読むとなったときに、どういった条件で分岐をして何を返しているのかわかりません。
修正
private String manpukuStatus(int amount) {
if (amount > STATUS_FULL) {
return "満腹";
} else if (amount < STATUS_MAX_NORMAL
&& 10 < STATUS_MIN_NORMAL) {
return "腹八分目";
} else {
return "おなかが減っている";
}
}
どう良くなったか
if文での分岐やリターンなどが見やすくなりました。
true/falseをreturnするような形で記載されていないか
if (age == 20) {
return true;
} else {
return false;
}
何が悪いのか
boolean値を返すだけのための分岐が記載されてしまっています。
修正
return age == 20;
どう良くなったか
age == 20が成り立つときすでにtrueであることが確定しています。
このことは自明なので、冗長な記載にならなくて済みます。
一度しか使っていない変数を定義してしまっていないか
int status = jobStatus;
if (status == 0) {
return NORMAL;
} else {
return ERROR;
}
何が悪いのか
一度しか使っていないのに変数が定義されることになってしまいます。
修正
if (jobStatus == 0) {
return NORMAL;
} else {
return ERROR;
}
どう良くなったか
不要な変数の定義がなくなりました。
条件式はメソッド化する
if (age >= TWENTY_YEARS_OLD && sex = MEN) {
...
}
何が悪いのか
条件が簡潔な場合はあまり問題になりませんが複雑化すると条件を深く読み込まなくてはならなくなります。
修正
if (isAdultMen()) {
...
}
どう良くなったか
メソッドとして切り出すことで条件式に記載されているものをそのまま読めば条件分岐の細かい中身を知らなくとも理解がしやすくなります。
固定値を引数としたメソッドはメソッドを作る
mthod(MyEnum.FOO);
mthod(MyEnum.BAR);
何が悪いのか
固定値のため、処理内容が変わらないのにメソッドの中身を細かく見ないと処理がわかりにくいです。
修正
mthodFOO();
mthodBAR();
どう良くなったか
FOOやBARに関連する処理であることがメソッド名でわかりやすくなります。
mthodFOO/mthodBARも簡潔になります。
処理と定義がまとまっているか
function gohan() {
itadakimasu();
taberu();
function itadakimasu() {
console.log("itadakimasu");
}
function taberu() {
console.log("taberu");
}
gochisousama();
function gochisousama() {
console.log("gochisousama");
}
}
何が悪いのか
「gohan」を実際に呼び出した際には「itadakimasu」「taberu」「gochisousama」がコンソールに出力されます。
しかし、「gohan」を参照した人は「itadakimasu」「taberu」で処理が終わるように見えます。
実際には読み込めばわかるのですが、混乱をさせやすいです。
修正
function gohan() {
itadakimasu();
taberu();
gochisousama();
function itadakimasu() {
console.log("itadakimasu");
}
function taberu() {
console.log("taberu");
}
function gochisousama() {
console.log("gochisousama");
}
}
どう良くなったか
「gohan」を呼び出した際に「itadakimasu」「taberu」「gochisousama」が順に呼び出されて処理が終了することがわかりやすくなります。
また順序としても「gohan」での処理とその処理で使う定義で分かれているため理解がしやすくなります。
if文で同列ではない条件を比較していないか
if (isMen()) {
...
} else if (isAgeOverThirty()) {
...
} else if (isLivingInJapan()) {
...
}
何が悪いのか
男性であるか、女性であるか、未解答やその他であるかという観点
年齢が30歳を超えているかという観点
済んでいるところが日本であるかという観点
がごちゃごちゃになっています。
例えばここに年齢が20歳以上であるかという条件文が増えるとどんどん条件は複雑化していきます。
修正
if (isMen()) {
...
}
if (isAgeOverThirty()) {
...
}
if (isLivingInJapan()) {
...
}
どう良くなったか
それぞれどの属性によってif文が記載されているかもわかりやすくなりました。
そして何かの条件を追加したとしても、ある程度の明快さは担保できると思います。
無駄に処理を書いていないか
return new ArrayList<>();
何が悪いのか
Collectionsで提供されているメソッドを使っていない。
修正
return Collections.emptyList();
どう良くなったか
個別でリストを作成する必要はなくなります。
インスタンス化が不要です（Collectionsクラス側に定義されているstatic finalの空リスト）。
同一の比較ができているか
list.isEmpty();
list.size() == 0;
何が悪いのか
同じ意味であるはずだが、記述がぶれている
修正
list.isEmpty();
どう良くなったか
比較方法の統一で記載がわかりやすくなります。
引数が固定値の場合のメソッド
run(XXEnum.FOO);
run(XXEnum.BAR);
何が悪いのか
固定値の場合は別途メソッドを作成する
修正
runFoo();
runBAR();
どう良くなったか
何に対しての処理であるかわかりやすくなります。