Java初心者だった新入社員の頃、先輩にコードレビューで指摘された事を思い出してまとめてみた。
追記：本記事に関しては賛否含め、多くの有益なコメントを頂いています。本記事をお読みになる際は、是非コメント欄も併せてご覧下さい。
2018/04/26
コメントを参考に「何でも定数にしようとする」の見出し・本文を修正しました。@kagilinn さん、ありがとうございました。
2018/04/30
サンプルコードの判定バグってたので修正しました。@y_miz さん、ご指摘ありがとうございました。
コメントの誤記、用語誤りを修正しました。@scivola さん、編集リクエストありがとうございました。
不要なインスタンス変数を作ってしまう
インスタンス変数は状態が保持されるので、バグを作り込みやすい。
「これローカル変数でよくない？」ってよく指摘された。
インスタンス変数を作る前に、ローカル変数で実現できないか検討する。
メソッドの先頭で全ての変数を宣言しようとする
C言語から入った人によくある習慣らしい。
変数のスコープが不必要に広がる為、トレースが非常に面倒。
public String function() {
int count;
int max;
int min;
List fooList;
String hogeCode;
String fugaCode;
String ret;
// 処理いろいろ
return ret;
}
変数の宣言はスコープを意識し、必要になったタイミングで行う。
説明不足な変数名をつける
初心者あるある。ベテランでも意外と適当な人がいたりする。
初心者でやりがちなのは、型名を省略しただけのものや、書いた本人しかわからない連番など。
String str; // 文字列であることしかわからない
String code; // 何のコードかわからない
int a; // 酷いけど稀に見かける
File file1; // 謎の連番
File file2;
static final String MSGID_E0001 = "E0001"; // 定数名に値が入っている
String userName;
String messageCode;
int age;
File userListFile;
File companyListFile;
static final String MSGID_FILE_NOT_FOUND = "E0001";
ただし、何でもかんでも修飾たっぷりの変数名を付ければ良いというわけではなくて、
for文のカウンタやcatchした例外など、短い名前で良い場合もある。
変数名をどこまで詳しくするかは、スコープの長さによって決めると良い。
インスタンス変数や長いメソッドで使われるローカル変数には、詳しい名前を付けたほうが良い。逆に、変数のスコープが数行のブロックに限る場合は、短い変数名でも問題はない。
重要なのは、コードの読み手(数ヶ月後の自分を含む)の立場に立った際、「その変数に何が格納されるのか」が理解できるかどうかである。
変数の命名は簡単なようでとても奥深い世界で、
名著「リーダブルコード」でも、変数名についてかなりのページ数を使っている。
booleanの変数名にxxxflgを使う
指摘されるまで違和感を全く持っていなかったやつ。
xxxFlgだと、trueの時にどうなるのかが分かり辛い。
private boolean writeFlg = false; // どういう場合にtrue/falseとなるのかが不明瞭
private boolean writable = false;
booleanの変数名は命題にする。
変数名 == trueと書いた時に、文として理解できるものが望ましい。
public boolean isWritable() {
return writable;
}
上記のようなメソッド名とすることで、呼び出しの際にインスタンスが主語となり、英文として意味が通じやすい。
if (note.isWritable()) { /* ... */ }
よく使われるメソッド名には次のようなものがある。
- is + 形容詞
- can + 動詞原形
- has + 過去分詞
- 三単現動詞 (+ 名詞)
【参考】booleanの命名について、英文法の観点でまとめられている記事
http://kusamakura.hatenablog.com/entry/2016/03/03/boolean_値を返却するメソッド名、変数名の付け方
意味を与えずに定数化する
「リテラルのべた書きはダメだ」という指摘を受けたので、色々定数化してしまった例。
定数が意味を持っておらず、ただ文字を置き換えただけになっている。
private static final String HANKAKU_SPACE = " ";
private static final String BLANK = "";
private static final String COMMA = ",";
private static final String SLASH = "/";
private static final int ZERO = 0;
必ずメソッドの最後でreturnしようとする
最初に指摘された時は、何が悪いのか理解できなかったが、
自分が読む立場になってみるとよくわかった。
boolean isPrimeNumber(int num) {
boolean ret;
if (num < 2) {
ret = false; // 2未満は素数でない
} else if (num == 2) {
ret = true; // 2は素数
} else if (num % 2 == 0) {
ret = false; // 2以外の偶数は素数でない
} else {
ret = true; // 割り切れなかったら素数
double sqrtNum = Math.sqrt(num);
for (int i = 3; i <= sqrtNum; i+=2) {
if (num % i == 0) {
ret = false; // 割り切れたら素数でない
break;
}
}
}
return ret;
}
boolean isPrimeNumber(int num) {
if (num < 2) {
return false; // 2未満は素数でない
}
if (num == 2) {
return true; // 2は素数
}
if (num % 2 == 0) {
return false; // 2以外の偶数は素数でない
}
double sqrtNum = Math.sqrt(num);
for (int i = 3; i <= sqrtNum; i+=2) {
if(num % i == 0) {
return false; // 割り切れたら素数でない
}
}
return true; // 割り切れなかったら素数
}
第三者が、numに1が入ってきた場合をトレースする場合を考えてみる。
「悪い例」では、どこでretが書き換えられているかわからない為、読み手はretの状態を記憶したまま、最後のreturnまで読み進めなければいけない。
一方、「良い例」では、各判定でその場でreturnしている。
num=1のケースをトレースする際は、3行目のreturnまでを読めば良い。
この例は10数行のメソッドだが、50行100行のメソッドを「悪い例」のように実装すると、読み手に掛かる負担は計り知れない。
メソッドは、戻り値が確定した時点でreturnするのが望ましい
また、早くreturnすることで、ネストが深くなりづらいというメリットもある。
不必要にpublicを付けようとする
初心者の頃は、privateで済む変数やメソッドをpublicにしがちである。
まずはprivateで作り、必要ならprotected, publicと広げていくのが良い。
安易にxxxUtil, xxxHelper, xxxManagerなどの神クラスを創造する
役割がはっきりしない名称のクラスは危険だ。
作った時点では良いが、5年10年とメンテナンスされていくうちに機能を押し付けられ追加されて手がつけられなくなっていく。
xxxの部分が抽象的であればあるほどまずい。最悪なのがCommonUtilである。
10年以上前に作られたCommonUtilを見たことがあるが、4000行を越えていた。
まずはFactory, Builder, Writer, Readerなど、役割が限定されるような名称に変えられないかを検討すると良い。
コメントを全く書かない
実装することに必死だったので、コメントを全く書かずにコードレビューに持っていったら怒られた。
コメントはちゃんと書こう。
特に以下のようなものは、コメントで補足をしておくべきだ。
- 難解な処理や複雑な条件分岐
- コードから読み取れない特殊な経緯や意図
自明な情報をコメントとして書く
「コメントを書きなさい」という指摘を受けた後、一行一行コメントを書いたらこれも指摘された。
コードをただ日本語訳したようなコメントを一行一行書いてしまうと、逆に雑音となってしまう。画面内に収まる有効行が少なくなる為、見通しも悪くなる。
// ユーザIDを取得する
String userId = user.getId();
// ユーザIDをリストに追加する
userIdList.add(userId);
ループ0回の考慮をしていない
for文やwhile文などのループ文を実装する際、ループが1回も回らないケースを想定しておらず、ユニットテストの0件パターンでバグが発覚するという事が何度かあった。
Foo foo = null;
for (int i=0; i < hogeList.size(); i++) {
if (i == 0) {
foo = new Foo();
}
// 処理
}
foo.function(); // ループ0回の場合はNullPointerExceptionが発生
ループ実装の際には、0件、1件、複数件のパターンを想定する
メソッドの戻り値・例外の設計を疎かにする
publicな共通系のメソッドにも関わらず、
引数や戻り値の仕様が曖昧だった。
例えば次のようなことは、漏れなく設計してJavadocに記載した方が良い。
- 引数でnullや負の数を与えられたら何を返すか
- どの場合に何の例外をthrowするか
- 戻り値としてnullを返すのはどのような場合か
Javadocの模範例を知りたい場合は、OracleのJavadocを見てみると良いと思う。
馴染みのあるStringクラスのメソッドなどがわかりやすいだろう。
さいごに
新人プログラマが良いコードを書けるようになる為には、本人の努力はもちろんだが、
優秀なプログラマからコードレビューを受けられるかどうかが重要だと思う。
レビュアからの「自分ならこう書く」というアドバイスを受け、「ああ、そういうのもあるのか」という気付きを得る。その積み重ねを経てノウハウを吸収していき、より良いプログラムが書けるようになっていくからだ。
コードレビューを教育の場とすることには賛否あるが、
私個人の意見としては、コードレビューは品質向上の取り組みであると同時に、勉強会であってもよいと思っている。
自分の書いたコードに対し、他のプログラマから指摘やアドバイスを貰い、時には議論する。そのような機会はコードレビュー以外ではそうそう無いだろう。バグ指摘だけで終わらせるにはあまりに勿体無いのだ。
途中でreturnがどうこうとか宣言を先頭でするとか(C#)
条件分岐は早期リターンを使用するか、無難に条件網羅で書くか-15[C#リファクタリングサンプル]
Java は全然知らないのですが，素数判定のコードの
は，ループを回るたびに
Math.sqrt(num)を評価することにならないでしょうか？@scivola ご指摘ありがとうございます。おっしゃる通りで評価されますね・・・ イケてないので修正します！
Javaは専門外なのですが、この部分は冗長では？
👇
@soi ご指摘ありがとうございます！前の文脈と絡めて意図的に書いたのですが、改めて読み返してみると英文としても冗長ですね… 修正します。
アノテーションも積極的に使っていきたいですね．
Androidならサポートライブラリ，Pure Javaでも JetBrainsがアノテーションライブラリを公開 しています．
のように，引数や返却値の条件をアノテーションで表現できます．
IDEによっては
@NonNullが付加されたメソッドに Nullable な変数を渡すと警告を出してくれます．@tetsukay この手のライブラリはLombokぐらいしか使った事がないのですが、IDEと連動してくれるものもあるのですね。便利だ・・・
アノテーション使うと、自然とnull意識したコーディングになるので良いですね。
プロジェクト標準やIDE標準のルールでの、
が守られてない限りレビューする価値なし。扱いにしてました。
レビューする上でコードの表現が定量化されているのは非常に効率的だと思います。
「何でも定数」については, 少し切り口が違う気がします.
定数を作り過ぎたのではなく「定数の意味を無視して定数化」しているのが問題です.
SLASH = "/" は良くないですが, これが URL_SEPARATOR = "/" であれば, 話が違ってくると思います.
必要なのは「意味づけ」だったのでは.
文字単位での定数（読み取り専用変数）は特定の文字であると明示する必要がある場合には意味があったりしますね。
全角ハイフンやらダッシュやら水平線やらを取り違えると障害になるような場面はままあるので、そういう箇所では敢えて
new String({(byte)0x0042})のように宣言したりします。@kacchan6@github フォーマッタや静的解析はコードレビュー前にかけるルールにしておきたいですよね。
インデントのずれとか不要インポート文の指摘で時間費やしたくはないものです。
@kagilinn
これは仰る通リですね。
何が問題かを取り違えていました。
@felis
似た文字の取り違え防止に使うというのもあるのですね。
そういえば以前、0(ゼロ)とO(オー)の区別が付きづらいフォントで設計書が書かれていて、
バグになったケースがあったのを思い出しました。
途中でリターンはスパゲッティーになりがちなんだけれども、その辺はどうなんやろ？
メソッドの先頭で全ての変数を宣言しようとする
命名規則とに絡むんだけど、そのメソッドで使う変数は先頭で定義してる方がやっぱり好み。
ループのインデックスとかはその場、その場で良いんだけど。
てか、このレビュー見てるとスパゲッティにならん？って気がする。
私は途中リターンでスパゲッティになったことないですし、
それでスパゲッティになるならメソッドの設計が悪くないですかね。
ある条件を満たしていないのであれば、これ以上処理する必要はない。ってことなんですが、
この手のふるい落としは大体メソッドの先頭の方でやるので、
メソッドの本体部分の条件分岐が減ってかえってシンプルになります。
仕様書でも条件と処理は分けて書いたほうが読みやすいのと同じだと思います。
ブロック条件でのリターンや例外スローは積極的に使うべきですが、ネストの深いループ等からの脱出は危険サインですね。
関心事の分離が十分にできていないかもしれません。
変数がメソッド頭で宣言されてる方がいいという意見が出るのも、似たような処理がネストしたブロック毎に書かれている場合にブロック毎に変数を宣言されると「これさっきと同じ名前だけど型も同じだっけ？」という余計な心配事が生じてくるからなので、設計を見直すべき時かもしれません。
変数の宣言をどこにおくべきか？という問が出てくる時点でメソッド全体が必要以上に長過ぎる可能性が大きいですし。
本当にすみません。ケチをつけるつもりはないのですが、
私も
SFyomiさん、felisさん同様
・メソッドの先頭で全ての変数を宣言しようとする
・必ずメソッドの最後でreturnしようとする
には物言いが。。大前提として記載のような問題が起きるのは
・１メソッドのステップ数が多い
・ネストが深い
場合であって、それはそれ自体が諸悪の根源であって、
・１メソッドのステップ数が10～40程度
・ネストは１～２の深さまででかつif文はほぼ使用しない
というメソッド設計をしていると
メソッドの先頭ですべての変数（一時変数を除く）を宣言し、バリデーションに引っかかった場合を除いてreturnは必ず最後に行う。
という形の方が全体の見通しはよくなるように思います。
※returnを必ず最後にするというのは、戻り値確認する際にトラップが張りやすいというのがあったのですが、抜ける条件と抜けさせる場所次第で早々にreturnさせる方がよい場合もあることは事実だと思います。
なお、「必ずメソッドの最後でreturnしようとする」の悪い例は、retをtureかfalse で最初に初期化をして開始することでぐっと見やすくなります。
P.S.
double sqrtNum = Math.sqrt(num); for (int i = 3; i <= sqrtNum; i+=2) { if (num % i == 0) { ret = false; // 割り切れたら素数でない } } ret = true; // 割り切れなかったら素数必ずret=trueになるのでふぐあっているような気が。。
double sqrtNum = Math.sqrt(num); ret = true; // 割り切れなかったら素数 for (int i = 3; i <= sqrtNum; i+=2) { if (num % i == 0) { ret = false; // 割り切れたら素数でない break; } }とかでしょうか。
「途中で抜けるのか最後に抜けるのか」に絡んで, Swift の guard 分を思い出しました.
Swift には if とは別の分岐として, guard 条件式 else 〜 文があります.
この文では 条件が成立したら次の行からの処理が続けられますが,
成立しない場合は else の内容が実行されます.
但しこの else 内では必ず return / throw (ループ内なら, break / continue も) などをして, 同じブロック内の以降の処理が流れないようにしなければコンパイルエラーです.
if の使い方のうち「そこで return して抜ける」タイプの使い方向けに別の構文を用意した感じがします.
guard と先頭に書かれた時点で, そこはジャンプ系の処理で終わるのが明確なので, それまで別言語で if 内で return していた場面は基本的にguard で書くようにしています.
何人かに否定されていますがこれは推奨したいです。
ループのカウンタがOKならif文内部でしか利用しない変数はどうしますか？すべてメソッドに切り出すならいいですが。
それだけは内部で宣言というなら統一性が保たれなくなりますよね？
また、スコープの問題もそうですが、基本的に変数宣言と同時に代入を行うほうが良いとされています。
Nullで変数宣言を行ってから再代入を行うと以下の問題が発生します。
ここら辺の問題と、イミュータブルであることが潜在的バグを生みづらいという考え方からすべての変数をfinalで宣言するという意見もあります。
とりあえず、関数の頭で変数を宣言して、リターンを最後にするのを書いてみた。
@7tsuno さん
私の個人的な見解で申し訳ないですが、回答させていただきますと、
そのうえで、途中途中で変数宣言があると、名前被りに気が付かないで不具合を引き起こすことがあったり、別の人がそのメソッドを後から手を入れる際に同じ名前で違う使われ方をしている変数があると、同じものなのか違うものなのかの判断に迷ってしまうのが大きいなと思っております。
結局はメソッド設計がよければ内部で使用する変数自体が少なくなって、先頭だろうが途中だろうがどっちでも問題なかったりするとは思いますので、だったら先頭にまとめとけばパッと見て何が使われているのかわかるのはいいんじゃないかな？程度かと思ってます。初期化忘れている変数もそこだけ見たらよくなりますので。
「メソッドの先頭で全ての変数を宣言したほうがいい」という意見が結構あって「マジか！」と思いました。
個人的には以下の順で考えると見通しが良くバグの混入も少ないと思います。
retが消えるなら一番いい）そもそも変数自体が無ければ無いほうがいいです。変数があること自体、自分の記憶領域を使うのでメソッドを読んでいく中で脳に負担があると感じます。
先頭に
boolean retという変数があると「こいつに何か再代入されるんだな」と構えてしまいますし、var ret = trueという変数であれば先に加え「なにがtrueなんだ？」と更に負担があります。更には変数retが「戻り値を意味する」ということを最初の行で察することすら負担です。無くせるなら無いほうがいいです。次に変数の作成が不可避（もしくは作ったほうが見やすい）の場合はイミュータブルにします。変数の再代入が可能な場合、常に変数がどのような状態か覚えていなければならないので一番脳に負担があります。同時にイミュータブルであれば予期しない値をとらなくなるのでバグの混入の可能性が減ります。
また、変数をイミュータブルにした時点で先頭での宣言は実質不可です。
最後にミュータブルにするならスコープを限りなく狭めます。基本的にループ内での利用になると思うので、ループ内で宣言すれば自ずとスコープは狭まるはずです。
スコープを狭められないよ！という場合はそもそもイミュータブルにすべき変数だったり、さっさとreturnできるパターンだと思います。
言語によって再代入を抑制できないものも多いと思いますが、イミュータブルなつもりで書くだけでも大分読む時の負担が違うと思います。
最後にリターンって、例えば
String firstname、String lastname、String nicknameのどれかがnullの場合は何もしないでエラーを返す。nullではなければDBに入れるというメソッドを作る場合、みたいに書くんですかね。もしくはネストしないように書くとすると、
ということですよね。DBに入れる条件としては厳密に言うと「エラーがなければ」ではなく「引数に
nullが含まれていなければ」なので意味の伝わるコードとしては不適切かもしれません。こうやって書いたほうがシンプルじゃないですかね。
引数にnullが入っていれば、もうこれ以上コードを追う必要がないですし、後続の処理で引数がnullなのかチェックする必要はもうないです。
なんだか長く盛り上がってますけど、メソッドの書き方はよほどヘンテコでなければ細かい部分は気にしなくてもいいのかなという気持ちもあります。
Javadocにちゃんと入出力と副作用と例外の条件が書いてあってその通り機能するのであれば、成果としては「ちょっと雑なコード」も「非の打ち所がないコード」もあまり差がないので。
特に初心者はコーディングに手一杯になりがちですので、コードの書き方は少し甘めに見て、オブジェクト指向の体得（まずは何でもstaticからの脱却）や、使い易いUIやAPIの設計、モデリングといったもう少しレベルの高い技能に目を向けてもらう方がいいのかな、と。
のコメントに対する意見ですが、
これは悪い例えではないでしょうか？
基本的に関数内の処理を続けられないError処理は即時Exceptionをthrowすべきかと。
個人的には、result変数は上書きしないというルールを決めることで、
result = となっている行は実質retun;を行っている位置と読み替えることができますし、
最終行の直前にブレイクポイントを仕掛けることで、デバッグ効率も上げることができるので、
resultが確定した時点でreturnするメリットを感じたことはあまりありません。
個人的には、好みの問題でどちらでもよいと思っているので、
レビューで強制的に修正させるほどのことではないように思います。
JAVAなんて10年、下手したら20年使ってないので、新しい仕様は全然わかってません。
仕事で使ってるのもVisual Studio 2005（のC++）なので、ミュータブルとかミューテックスの親戚？なんてググってみたり。
仕事で使ってる身としては、全体で統一されていれば、なんでもいいよ、と思ってます。
まぁさすがに説明不足な変数名やコメントがないとかコメントがウソとかはやめてほしいですが、統一されてればなんとかなる。