コードは他人が読む前提で書いてほしい
今やってる案件のコードが、あまりにも汚くてびっくりしています。
元々うちが書いたものではなく、他社(超有名どころ)が製造したコードを引き継いだものです。
うーん、1年生が書いたコードをコードレビューは一切せずにリリースしたのかな？？
それとも他の言語(恐らくC言語…？)しか書けない人が書いてコードレビューは以下略？？
無駄にトリッキーだし、そもそもよくこれで動いてるな？？？ってのが正直な感想。
なので修正対象のクラスについては元のコードと差分比較する意味がないくらい丸ごとリファクタリングして、JUnitも全部書き直すことにしました。…改修でバグ埋め込んじゃいそうだし…
実際、細かく読んでたらバグをいくつも見つけてしまいました。まあJUnitも穴だらけだから仕方ないのかもしれないけども。よくこれでリリースできたな！？
そんなわけで上から「リファクタリング例とか傾向とかまとめて資料作ってね」って話が来たので、せっかくだからJavaを書く時には意識してほしいな
って思ったことを残しておきます。
内容はふんわり、新人プログラマさん各位向けです。というかベテランは多分無意識でやってると思う。
**見出しの記載順は、思いついた順なので、優先度順ではありません。**全部意識してほしいです。
仕事で書くコードは、自分だけではなく他人も読むものだと最初から意識しましょう。
そして自分で書いても、時間が経ったら別人が書いたものと同じように読みにくいものです。
リリース・瑕疵対応あたりまでは面倒みるとしても、システムの終焉までメンテや改修を1人で担当できるって言いきれます？
従うべきコーディング規約が存在するプロジェクトでは、それを最優先します。
コーディング規約は遵守が基本姿勢です。
更新履歴
- 2018/12/18
- 2018/12/20
- 「意図が汲めない命名は悪」を追加
- 「おまけ2(参考文献や読み物)」で参考とかいろいろ追加(多分これからも随時)
- 2019/03/26
- 「統一されないコーディングスタイル・フォーマットは悪」を追加
- 「コメントアウトで残したままの処理は悪」を追加
- 「宣言とか代入での桁揃えは悪」を追加
- 2019/09/19
- コードの例や例外的な注意事項を追加、いろいろ調整
- 2020/02/25
- 「車輪の再開発は悪」を追加
- 「知識のアップデートを怠るのは悪」を追加
- その他いろいろ見直し
- 2020/08/19
- 「意図が汲めない命名は悪」にハンガリアン記法についてを追加
意図が汲めない命名は悪
**めちゃくちゃ大事。**可読性に直接つながります。
業務で成果物として書いたコードは、基本的に自分以外も読みます。改修します。
その時に内容を察せられない命名をされると、大抵詰みます。読めねえよ！ってブチ切れそうになります。
適切な命名は可読性を爆上げする
例えば、Stringの変数を3つ宣言するとします。じゃあ使う順番にstr1、str2、str3……
パッと見たときにstr1って何！？ってなります。型しかわからない。~~型なんて宣言見りゃ一発で分かるだろうが。~~何も情報がないのと同じです。
こんな名前では、1か月後どころか1週間後の自分だってわからなくなる可能性があります。
自分の書いたコードすら読めないエンジニアって、かっこわするすぎじゃないですか？？
ちゃんと内容がわかる名前を付けましょう。
Javaの命名のお作法
基本的にJavaでは以下のように命名します。コーディング規約で指定されている場合は、それに従いましょう。
たまによく見かけるんですが、変数名がスネークケースとキャメルケース混在しているとか、よくないです。せめてクラス内で統一しましょう。
英語とローマ字を混ぜるのもよくないです。
「createChohyo」とか。「createReport」でいいよ。なんで「帳票」をローマ字にするんだ。わからない英単語はググれ。
DBの項目を格納する変数でDBのテーブル定義がローマ字混じり、などのケースは除きますが。
- クラス名はアッパーキャメルケース(先頭が大文字のキャメルケース)(Pascal形式ともいう)
- Ex：StringBuilder、IndexOutOfBoundsExceptionなど
- メソッド名、変数名はキャメルケース(先頭が小文字、つながった単語の先頭が大文字)
- Ex：isEmpty、compareToIgnoreCaseなど
- 定数名はアッパースネークケース(すべて大文字でアンダーバーで単語をつなぐ)
- Ex：WEEK_OF_YEAR、DAY_OF_WEEK_IN_MONTHなど
ハンガリアン記法について
ハンガリアン記法 - Wikipedia
プログラマがプログラムのソースコードを書く際に変数名やクラス名などの識別子に特別な接頭文字ないし接尾文字をつけることで、他の人がその識別子を見たときに識別子の使用方法・データ型情報・スコープなどが分かるようにするための命名法である。
プロジェクトのコーディング規約に記載がない場合、システムハンガリアンはやめましょう。時代遅れです。
有用なのはIDE未使用の案件くらいですかね…でもJavaの業務でそれは現在はそうそうないと思います。
そもそもJavaは静的型付けの言語ですし。それに型を変えたら変数名まで変えないといけません。
あとメソッド引数のシステムハンガリアンも冗長です。IDEやVSCのようなエディタでは出現箇所がわかるので。
アプリケーションハンガリアンならまだ有用です。
でも判別のためのプレフィックスなりサフィックス自体がわかりやすいならともかく、プロジェクトや業務の固有名詞など、初見の人がわからない略称になる場合は、きちんと変数名を付けてください。
例えば編集前と編集後の値を判別するためのbfrとaftとか、そのくらい汎用的なものなら有用だと思いますが。
例外的な命名
スコープの短いごく一時的な変数ならStringのstrとかintのnumなどにすることはあります。拡張forループの変数とか。
ただ、これは短いスコープの中で変数の意図が汲みやすい場合にのみ許される話で、やっぱり基本的に命名はきちんとするほうがいいです。
ですが、用途の限定された慣習的な変数名はオリジナリティを出さないでください。
具体的に挙げるならば、try～catch句でのExceptionのeやex、forループのカウンタ変数のiやjなど。
それらでオリジナリティを出されると逆に可読性が落ちます。そこはそのままでいいです。
また、逆に他の用途や型の変数で慣習的な変数名を使うのもやめましょう。
具体的な命名の方法やtipsはQiita内にもたくさん記事があります。「Java 命名」とかで検索してみてください。
その時は複数の記事を並行して読んでください。単独の記事を読んだだけで理解した気になるのは、悪です。
冗長だったり可読性が低いコードは悪
分岐を減らす、命名を内容に沿ったものにするなど工夫しましょう。省略してもコメントが不要な単純なものは省略します。
よく見かけますが、何でもかんでも変数を宣言するのも可読性が落ちます。
1度しか参照しない短い記述で済む値を逐一変数に格納すると、その分だけ読むべき記述が増えます。スコープが広ければ尚更。
変数に格納しているならばその値はそのあとの処理で参照するから必要がある、と考えることが多いので。
記述が長すぎたり複雑になってしまうこともあるメソッドチェーンやメソッドの入れ子、文字列の結合などの場合、デバッグのしやすさも考えて適度に変数に格納するのがいいとは思いますが。
初学者のコードでよく見られる例
1クラス、いや1メソッド単位でも、行数が多い方がバグが出やすいです。また、単純にテストする量が増えて工数が嵩みます。
可読性の高いコードは、簡潔にまとまってることが多いです。つまりだらだら長いコードじゃないです。
他人が読んで読めない読みにくいコードは、バグの温床です。
調査とか改修時に高確率で担当者が地獄を見ます(真顔)。
真偽値の判定
確かに想定通り動くけど、if文で「== true」、「== false」を記述するのは正しくありません。
Javaのif文は真偽値のみを評価するため、下記のダメな例はゴミを付けただけです。真偽値の判定をネストしてどうする。
// 冗長なif文の条件判定
// ダメな例
if (条件A == true && 条件B == false) {
}
// いい例
if (条件A && !条件B) {
}
変数宣言
下記は、変数の宣言と値の初期化について、よく見かけるダメな例です。
初期化を上書くだけの代入を別ステップにするのも、意味のない冗長なコードになります。
// 冗長な変数宣言 宣言時の値を単に上書きするのは意味がない
// ダメな例その1 必ずnullで初期化する必要はない
List<String> list = null;
list = new ArrayList<String>(Arrays.asList(new String[]{"ABC", "123"}));
// ダメな例その2 初期化は必ずnewでする必要はない
List<String> list = new ArrayList<String>(0);
list = new ArrayList<String>(Arrays.asList(new String[]{"ABC", "123"}));
// ダメな例その3 宣言と初期化を個別にする必要はない
List<String> list;
list = new ArrayList<String>(Arrays.asList(new String[]{"ABC", "123"}));
// いい例 代入するなら宣言と同時に、無駄な初期化はしない
List<String> list = new ArrayList<String>(Arrays.asList(new String[]{"ABC", "123"}));
if分岐の最適化
上記の変数の宣言と初期化に絡めて、変数への代入自体が分岐する場合を例とします。
その場合、宣言と代入を別ステップにする意味がありますが、2パターンしかない場合はif～elseではなくifだけで書けます。
分岐が3パターン以上ある場合も、if文の最適化を考えましょう。
列挙型による分岐であれば、switch文にするのも有用です。
// 代入自体が分岐する場合(分岐が2パターンのみ)
// 冗長な例
String str = null;
if (条件A) {
str = "ABC";
} else {
str = "123";
}
// 不要な分岐を削除した例
String str = "123";
if (条件A) {
str = "ABC";
}
// 三項演算子を使用する場合
String str = 条件A ? "ABC" : "123";
上記のようなコードは、条件Aやstrへ代入する値の導出が単純でない場合は、String str = 別メソッド呼び出し;の1行にできるので、戻り値がstrの値となる別メソッドにするのもいいと思います。
1か所のみのあまり複雑でない処理を別メソッドにする是非は、コードレビューアによって変わるかもしれませんが。
コーディング規約で禁止されてない限りにおいては、単純な条件で値を変える場合、三項演算子の使用もおすすめです。
ただし、三項演算子の使用は単純な分岐に留めるべきです。条件が複数あったり条件や分岐がネストする場合は、逆に可読性が落ちるのでやめましょう。
車輪の再開発は悪
既に存在する機能を、自分で実装するのはやめましょう。
その製造とテストは、本来ならば払う必要のない無駄しかないコストです。無意味どころかマイナスです。
ありがちな具体例
すごくよく見かけるんですけど、apache.commonsのライブラリが使用可能なのに、標準APIだけを使っているとか。
ひどいと共通クラスを切ってまで標準APIの組み合わせでメソッドを作りこんでいるのを見ます。
もちろん、commonsライブラリのバージョンが低くて適切なメソッドが存在しないとか、そもそもcommonsライブラリが使用不可とか、そういったケースがあるのも身をもって知っていますし、そうであれば仕方がないことなのですが。
使えるのに使わない、車輪の再開発を自ら進んで行うのはダメです。
また、プロジェクト内に共通クラスがあってそこで宣言しているのに、それを使用しないのもダメなやつです。
// 文字列がnullまたは空文字のチェック
// ダメな例
if (str == null || str.isEmpty()) {
}
// よい例
if (StringUtils.isEmpty(str)) {
}
// 文字列がnullかトリムして空文字だったら指定の文字列に変換
// ダメな例
String str = value;
if (StringUtils.isBlank(str)) {
str = "未設定";
}
// よい例
String str = StringUtils.defaultIfBlank(value, "未設定");
// オブジェクトのnullチェック
// ダメな例
if (obj == null) {
}
// よい例
if (Objects.isNull(obj)) {
}
面倒くさがらない
WEB検索したり、使っているライブラリの中にその機能が存在するかどうか、調べるコストって結構かかります。
でも、それを面倒くさがっての車輪の再開発は、プラスどころかマイナスです。やってはいけません。
コードを記述したら、記述した分だけテストをしなくてはなりません。
既存の機能を使えば、その部分についての詳細なテストは不要になります。
commonsライブラリであれば、世界中で使用されていて、バグが上がってない時点で動作が保証されているのも同然です。
むしろ正しく使ってバグが発生するのを発見してOSSコミュニティに報告したら、すごい功績です。胸を張って自慢できるレベルです。
ですが、自分で記述したらすべてテストする必要があります。誰も動作を保証してはくれないので。でもそのテストは無駄なことです。
出先で行きたいお店の電話番号を調べるのに、スマホを持っていたらスマホで調べますよね？電話ボックスに入ってタウンページ引きませんよね？ましてや電話番号案内に電話かけないですよね？そういうことです。
知らないことは悪いことではありません。悪いのは、知ろうともしないその怠慢な態度です。
せめてプロジェクト内は調べましょうよ。自分で調査してから有識者に確認をします。(自分で調べる工程を省いてはダメ)
必要な機能が存在していなくても、存在しないということがわかります。それならば有識者に共通機能として製造するか、それとも自分の担当箇所だけに記述するか確認します。
具体例で出しているcommonsのライブラリは、バージョンによっては存在しないメソッドもあります。
WEB検索で使用例が見つかっても、担当案件で使用しているバージョンに存在しないメソッドもあるでしょう。でもそれはそれで知見が増えます。
英語ドキュメントは怖くない
調べようとしたけどJavadocが英語で読みにくい場合は、コードを直接読んでみましょう。そのほうが早い場合も多々あります。
commonsのStringUtilsクラスなんかは、難しいことは特に記述されていません。初学者でも読めるメソッドが大半です。
メソッドの命名もわかりやすいものが大半です。担当案件で使用しているjarのコードを直接IDEで開いてみましょう。
Eclipseを使用している場合、アウトラインビューを使ってメソッドをABC順に並べたりすると、目当ての機能が探しやすいかもしれません。
学校の試験ではないので、きちんと和訳する必要はありません。ざっくり意訳・直訳ができればいいのです。私もGoogle翻訳頼りです。でもそれで意味が分かるなら大丈夫。使えるものは使いましょう。
文中のそれっぽい単語を拾い読みするだけでも意味は掴めたりもします。各種Exceptionなんか特にそうですよね。
やる前から投げるのだけは、よろしくありません。
知識のアップデートを怠るのは悪
この仕事を続けてくのであれば、知識のアップデートは必要不可欠です。
自分の覚えた知識だけで仕事したいなら、この仕事は向いてないです。
『今』に必要な知識を得る
案件によって使用可能なライブラリは変わります。毎回そのすべてを覚える必要はありませんが、調べることは必要です。
脳死で製造する人もいるんですが、技術的負債の蓄積に他ならないので、やめてください。書くべきではないコードを積み重ねないでください。
きつい言葉ですが、毎回解析に苦労しては大量のコードのリファクタリングをさせられる身にもなってみてほしい。(そういうポジション多いんです)
Javaの日時管理ってDateとCalendarからLocalDateTimeに代わりましたよね？
File操作もjava.nio.fileパッケージの登場で随分楽になりましたよね？
最近実感するのは、try-with-resourceもラムダ式もStream APIもOptionalも一切見かけないJava8以降の新規構築の案件。
…メンバーの知識が止まっているんですよね。多分このパターン官公庁とかインフラ系の請負に多いと思うんですが。
でも、新卒もメンバーにいるはずなのに。周りのコードがレガシーだから、レガシーなコードを書いちゃう。よくない傾向です。
私もつい最近まではJava8っぽいコードが読めませんでした。Java6までのレガシーコードの案件しかなかったので。
ドキュメント類を読んでも理解が追い付かなかったのです…実際に書いて動かさないと無理…使う必要がない知識って、覚えられない…
でもJava8の案件にアサインされてしまったので、覚えようと意識しました。おかげで半月経つ頃には、for文を書くことがほぼなくなった程度にはStreamAPIに慣れました。Google先生様様です。
Java8のコードが読めるようになったので、できることも増えました。もう検索結果でラムダ式のサンプルコードが出てきても怖くありません。回れ右しなくてよくなりました。(笑)
知らないことを知って、できることが増えるのは、何歳になってもうれしいものですよ。
単体テストが実施しにくいコードは悪
例えばJUnitテストケースが書きにくいコードですね。
長く複雑なメソッドは、テストケースを考える・記述するコストが嵩みます。メンテも改修もしにくいです。
主となるメソッドの処理を小分けにし、小分けにした処理を別メソッドとして分割するなど、同じ処理内容でも別の記述方法を考えましょう。
改修で完全新規クラスのpublicメソッド1つで300行越えだったのを見たことあるけど、流石にそれはお客さんからクレームが来たって聞きました。
2019年のJavaの話です。古いCじゃないですよ。
もちろん処理のネストも深く、可読性はとても低かったです。
書いた本人以外はJUnitなんて書けないレベル。手伝わされたけど、本人からの解説なしには無理でした。
でも、privateとして分割したメソッドのアクセス修飾子をJUnitで呼び出すために削除するなんて愚行は絶対やめるべき。
テストのためにテスト対象を正しくないあり方にする。そんなのは本末転倒に他なりません。
本当によく見かけるんですけど。
アクセス修飾子の制限によりテストクラスから呼び出せない場合は、リフレクションを使用します。
テスト対象メソッドをprotectedにし、テスト対象クラスを継承したテストクラスから呼び出す…とかもダメです。
protectedはテストのためにあるアクセス修飾子ではありません。
private staticなメソッドでもリフレクションで呼び出してテストは可能です…
追記1
privateメソッド・フィールドを呼び出すJUnitテストクラスについて記事を書きました。
JUnitでprivateメソッドのテストを行う
リフレクションでprivateな変数を参照・設定する
追記2
Javaではアクセス修飾子は必履修なので、わからない場合は調べてください。
JavaScriptやPythonなどアクセス修飾子がない言語も存在しますが、Javaはアクセス修飾子が存在する言語です。
PythonやJavaScriptにアクセス修飾子がない理由を考えてみた - Qiita
深すぎるネストは悪
上でも書きましたけど、ネストが深い(条件分岐やループのブロックが多段の入れ子構造になっている)コードは、複雑で可読性が低いし、JUnitも書きにくいです。
そんなネストが深いコード、それは処理を分割して別のメソッドにするサインでもあります。
可読性を考慮しないコードはテストもしづらいですし。
アーリーリターン、ガード節の利用
例えば、単純なifのネストは条件を反転することで減らせたりします。
よくあるパターンでいうと、引数が空じゃなかったら処理するけど、空の場合は決まった値を返すとか。
その場合は、条件を逆にして、処理しないパターンでは先に処理から抜けるように記述します。
// 悪い例
boolean result = true;
if (条件A) {
// 条件Aの場合の処理
// ………
} else {
result = false;
return result;
}
return result;
// いい例
if (!条件A) {
return false;
}
boolean result = true;
// 条件Aの場合の処理
// これ以降もアーリーリターン可能な時点で戻り値を返す
// (以降の処理で戻り値が分岐しないのであれば、result変数の宣言は不要)
// ………
return result;
上記の結果は同じです。でもこれだけで条件Aのブロック内で行う処理におけるネストがひとつ減ります。
条件Aのブロックが複数行の場合、条件A以外はどうするのか、条件Aのブロックを読み終わるまでわからないのも可読性を下げています。
for文の中で条件分岐する場合も、このパターンでcontinue/breakしてネストが減らせたりします。
また悪い例の場合、elseの戻り値が変数になっているため、戻り値が何かがパッと見で分かりません。
false以外のパターンがないので、return false;でいいのです。return後の処理はないので、変数への格納は不要です。
また、if文の前に宣言する必要もなくなるため、変数のスコープも狭められます。
複数個所で個別に記述された同じ処理は悪
同じ処理を個別に複数個所で書くことの何が悪いのか。それはメンテや改修が大変になるからです。
例えば、ある処理を複数個所で実施していますが、すべて個別に記述してあるとします。
その処理を改修することになりました。すべての箇所について同じ内容で確認・改修・テストが必要になります。
同じ記述の場合、コピペで修正することもありますね。
修正箇所が増えれば、確実にコピペミスが発生します。コメントや変数名の変更漏れなど、よくあることです。
また、その処理の実行箇所に過不足がないか、すべてのコードに対して確認が必要です。
クラス内で済めばいいですが、共通化していないことによって、プロジェクト全体を確認する羽目になったりします。
繰り返し行うコストのすべて、本来なら省ける無駄なものです。
同じ処理を行う記述は、ひとつにまとめて別メソッドに切り出しましょう。呼び出し箇所で異なる値は、引数で渡せばいいのです。
ザーッと動くコードを書いてから、リファクタリングするのをオススメします。
JUnitも書いて処理内容が正しいことを確認してからリファクタリングすると、より安心安全ですね。
一度書いていれば、「ここの処理って前に書いた気がするぞ…？」って気付きやすいでしょう？
但し、上記は新規に製造する場合であり、例外として既存システムの改修においてはやむを得ない場合もあります。
例えば、同じ処理が複数のクラスで個別に記述されている既存システムにおいて、共通化すべきクラスの一部に改修対象となるクラスが含まれている場合等です。
既存システムの改修では、変更の必要がない箇所について可能な限り修正しないことが多いです。変更したクラスは粒度やフェーズはさておいて何かしらの再テストが必要で、そのための工数が増えるからです。
そのような場合、改修内容に関係のないリファクタリングを行う方針になったとしても、改修対象クラス内のリファクタリングだけに留め、別クラスに共通のメソッドを切り出すようなリファクタリングは行わないとするケースが殆どです。
既存システムの改修時は、リファクタリングの可否と範囲を上長や有識者に確認しましょう。
技術的負債となるようなリファクタリングの必要なコードは、(そもそも)(可能な限り)リリースしないのが一番なんですけどね。
意味もなくスコープが広い変数やメソッドは悪
JavaはJavaであって、CでもC#でもJavaScriptでもない
まず、C言語習得者は、Javaを書くときはC言語を忘れてください。C言語とJavaは別物です。
JavaとC言語は、見た目(コーディングスタイル)はちょっとだけ似ているかもしれません。ですがC言語とJavaは別です。
JavaとJavaScriptの名前だけが似てるくらいの違いです。中身は完全に別です。一緒にしないでください。
**C言語が書けるからと言ってJavaは書けません。**一緒くたにするのは本当にやめてください。
C言語はオブジェクト指向ではありません。別物です。共通点は見た目だけです。
現役力士と橋本環奈が同じ服を着ていたら、同一人物だと判断しますか？しないですね？それと同じことです。
変数を宣言するなら、使用する場所で
メソッドの先頭で変数をまとめて宣言するのはやめましょう。
IDEを使っても普通のローカル変数は、型くらいしかポップアップに出ません。
宣言と実際に使う場所が離れていると、宣言を見に行く無駄な手間が増えます。
C言語(やVBA)から入った人にありがちですが、それはJavaにおいてはバッドプラクティスです。
変数は、使用スコープを意識して宣言しましょう。ブロック内でしか使用しない変数をブロック外に書く意味はないです。むしろ書いたらダメ。
たまに古い時代のコーディング規約使ったままのプロジェクトで、頭でまとめて宣言しろって書いてある場合もあります。
ですがそれは今のJavaではクソルールなので、場合によっては疑問の声を上げるべきですね。
スコープを意識する
スコープとは、使用可能な範囲と捉えてください。
Javaでは、ブロック内で宣言した変数をブロック外では使えません。(ブロックスコープ)
例えばfor (int i = 0; i < array.length; i++) {みたいなfor文、ここで宣言してるループカウンタの「i」ってfor文の外ではコンパイルエラーになりますよね。forブロックを跨いだ再利用もできません。
こんなその変数は、ブロック内だけを意識すればいいのです。意識すべき範囲が狭まるだけで、ぐっと可読性が上がります。
ブロックの外で宣言された変数は、ブロックを抜けた後も参照や代入をすることができます。
できる。そう、可能性です。可能性があるからその変数が使用されている箇所は全て意識する必要がある、しなければならない。
ブロックスコープの有効活用は、そこでしか使わないと明示的にすることです。読む人の脳のメモリも、そしてJavaVMのメモリも節約できます。
だから変数のスコープは最低限にしましょう。
変数の再利用はしない
変数の再利用は可読性を下げ、バグを生みます。
その変数が使われているところをすべてを意識する必要が出てきてしまいます。グローバル変数の多用が悪く言われる理由は、その管理のし難さです。
どこで代入してどこで参照するか。すべての箇所を把握するのは大変です。
また、再利用時には値の初期化が必要な可能性も出てきますが、その初期化を忘れたりしてバグを作りこむ可能性も上がります。
再利用する場合は変数名も汎用的なものにしがちですが、それもよろしくありません。
つまり、**変数の再利用は百害あって一利なしです。**省略してしなくていいどころかしてはいけない省略です。
昔のコーディングスタイルのことは忘れよう
昔はメモリ節約のためにループ中で使用する変数はループ外で宣言を、みたいな傾向もありました。が、今はそこまで気にしなくていいです。
ループ内でしか使わないならループ内で宣言してください。
ループの外で宣言すると、ループの頭で毎回初期化しなきゃいけないの忘れちゃったりして、バグバグしたりするよね！
まあ逆にループカウンタとか、ループの外で宣言しておくべき変数もあるんですが。
アクセス修飾子を意識する
アクセス修飾子にも、変数と同じことが言えますね。
メソッドでも変数でも、何でもpublicやアクセス修飾子なしにするのはやめましょう。
どこからでも触れることはメリットじゃありません。大抵の場合、デメリットです。
コードの単純なコピペは悪
内容を把握できていないコードは記述しないようにしましょう。
「よくわからないけど動く」状況を作り出しがちですね…わからないからコピペ、はやめましょう。
コピペする場合でも、ちゃんと読んで内容を把握して、コピー元コードをペースト先に合わせてリファクタリングすることが最低限必須です。
人に説明できないコードは書かない！
肝に銘じてください。
コードレビューで質問されて自分で説明できないコードは書くべきではないのです。
「検索で見つけたから」「こうやって書いてあるところがあったから」etc……これらは理由になりません。回答でもありません。ただの言い訳です。
脳死でコピペせずに、せめて何をしているか読んで把握くらいはしてください。わからないAPIはJavadocを読む習慣をつけましょう。
お作法的なものでも、そのお作法ではどんな処理をするのかくらいは読んでください。読まずに経験値は得られません。
参考と解答はイコールではない
たまに(新人じゃなくても)いるんですけど、「参考コードを教えてくれ」ってきて、教えるとそのコードをそのままコピペするの。
お前は「参考」の意味を辞書で一度引いたほうがいい。
そういう人に限って上手く動かないと、教えた人のせいにしてくるんですよね！！！！！！！！！
てめえの頭は飾りか？ああ？**自分のコードに責任を持てよクソが！！！！！**って悪態吐かれても仕方のない行動です。
処理を詰め込みすぎたメソッドは悪
特にpublicメソッド。長いpublicメソッドのテストクラスの記述は困難を極めます。
1メソッド1機能を意識しましょう。テストクラスも書きやすいです。あとメソッド名もつけやすい。
privateメソッドから更にprivateメソッド呼んでもいいので、なるべく複雑化しない方向で。擁する機能が少なければ、コードも短くなりますし。
最大でも100行/1メソッドくらいに収めるといいと思います。
個人的にはスクロールせずに1画面内に表示可能な行数を目安にしています。
処理を詰め込みすぎたメソッドの改修は、無意味にテスト範囲が広がります。
処理A、B、Cの3つの機能を1つのメソッドで処理していた場合、処理Aだけを改修しても処理B、Cが影響を受けてないことを確認するためのテストが必要になるでしょう。
Javaはオブジェクト指向なので、単一責任原則を念頭に置くといいです。
単一責任原則 | プログラマが知るべき97のこと
publicメソッドの中で、それぞれの処理を行うprivateメソッドを順番に呼び出していく…みたいな感じだと読みやすいですよ。
処理ごとにメソッドが分かれていれば、テストのしやすさはぐっと向上します。
プラモデルみたいなものと考えてください。腕パーツだけ塗装するなら、腕パーツを外せるようになっているほうが作業しやすいです。
ソフビの一体成型のフィギュアだと腕以外に色がはみ出ないように気を使う必要がありますから。
マジックナンバーのまま放置されたリテラルは悪
同様の意図を持つ同一のリテラルは、定数として宣言して1か所へ集約と意味を持たせましょう。
何度も同じ意図で使用する値をマジックナンバーのままにしておく理由はありません。
定数として名前がついていれば、「この値は何？」「何でこの値？」ってなることは確実に減ります。
また、定数に宣言して使用していれば、値が変更になった時は定数値を変更するだけです。複数個所で使用していた場合、かなりのメリットですよ。
基本的にマジックナンバーをそのままハードコーディングにしておくのはよくないです。
ただ、1回1か所でしか使わないリテラルをprivate定数にしすぎるのも可読性が落ちるので、ケースバイケースなところもあります。
例えば変数名がzipCdでsubstring(0, 3)やsubstring(3)している場合。
このくらいであれば郵便番号をハイフンの位置で分割しているんだなって見ただけでわかります。
ですが業務的な値であれば、定数に宣言して使うか、もしくはその処理用のメソッドにすべきです。
変更される可能性がある場合、定数ではなく外部プロパティファイルに外出しするのもいいですね。
プロパティから取得するときのキー名で意味を持たせられますし、コードを修正しなくて済みます。
コメント欄より追記
たまたま値が同じだけで用途・意図が異なるものは、共通の定数にしないでくださいね。
それはバグへの一歩です。用途や意図が異なる場合は別の定数で宣言しましょう。
何のために定数として名前を付けるのかを考えるのは、大事なことです。
Javadocがないメソッド、コードと一致しないJavadocは悪
最低限でも@param、@return、@throwsがソースと一致したJavadocは必須です。
JavaはIDE前提ですし、Javadocだけ確認したらそのメソッドを呼び出せるくらいの説明が書いてあるといいですね。
そうじゃないとメンテや改修時に呼び出し先の機能も全部読まなきゃいけなくなっちゃう。
他人の書いた見ず知らずのコードを読むコストは相当高い
例えば引数の内容を説明する@paramタグや戻り値を説明する@returnタグ。
引数や戻り値ががあるメソッドなのにタグ自体がないのは論外として、タグのみだとか@param strみたいに変数名だけ(自動生成しただけで放置か？)で説明がないパターンとかあるあるですけど、謝罪して即時修正してほしいレベル。
それでも型が特徴的だとか命名から察せられるならまだいいです。
int update(Connection con, String sql, Map<String, Object> params)とかなら、ぱっと見でコネクションとSQL文とSQL文に埋め込むパラメータ渡してDBを更新して件数返すんだなってわかります。(それでもJavadocの記述は必須ですが)
だけどString edit(String str, String value, boolean flg)みたいなメソッド、何のために何を渡して何をして何を返すのか推測できません。多分書いた人も時間経過で分からなくなるやーつです。
でも、Javadocが記述してあれば、中身を読むコストがほぼ不要になるんです。
Javadocは随時更新すべきもの
Javadocは、製造時に記述するだけのものではありません。
改修時に引数や戻り値が増えたり減ったり変わったりすることもあるでしょう。その場合は必ず更新してください。
コードと一致していないJavadocは、ゼロじゃなくてマイナスなんです。ゴミです。地獄の火の中に投げ込まれるべき。
せめて概要と引数と戻り値。この3つはその時点のコードと一致させてください。
コードを記述、修正したら必ずJavadocを見直す習慣をつけるのがいいですね。
設計書と完全一致している前提のコメントは悪
設計書の章番号などを含める等、コメントと設計書とリンクさせるのはやめましょう。
設計書の修正時に、項番が変わっただけで修正対象以外のコードのコメントまで同期させる必要が出てきます。
(それが常態なのがおかしいのですが)設計書をメンテしないプロジェクトも多いし、コードと設計書がどんどん乖離していきます。
そして改修時に「設計書はあてにならないからコード元にして設計書を最新化して」とか言われて無駄に工数増えるんだ…
詳細すぎるコメントは悪
複雑でない処理や、簡素な条件分岐ごとに逐一記述されたコメントは、可読性が落ちます。
コメントを削除したら行数が半分になる、そんなコードを書くのはやめましょう。
よく使う標準APIのjava.lang.Stringですが、一度読んでみてください。EclipseならF3を押すだけでソースに飛べます。
Javadoc以外で詳細なコメントが記述してある箇所は、それほど多くはないと思いますよ。
必要なコメント、不要なコメント
例えば、コードを見ただけで分かる「リストの件数が0件の場合」なんて条件、コメントが必要ですか？
コメントを書くときは、そもそもコメントが必要かどうかを考えましょう。
複雑な条件判定でも、わかりやすい名前を付けた別メソッドに切り出したら、コメント不要になりませんか？
不要なコメントの必要がないコードは、読みやすいです。
コメントを記述するためのリソース
大量のコメントは、コード修正の際に合わせて修正する手間もばかになりません。
更にコメント量が多いほど、修正漏れやコピペミスなどで処理と乖離した誤ったコメントも増えていきます。絶対です。断言できる。
間違ったコメントは、コメントがないよりも可読性を落とします。バグの温床です。
コード不一致のJavadocと同じですね。
統一されないコーディングスタイル・フォーマットは悪
(最低限)ファイル内では統一する
インデントのタブやスペース、波括弧{}の前後の改行、演算子や丸括弧()の前後のスペースなど。
調査でgrep検索かけるにしても、スペースの数を正規表現なりで考慮しないと引っかからないし、いいことはないです。
IDEを使っていれば自動フォーマット機能があると思うので、保存時にフォーマットされる設定にしておくのもいいですね。
コーディング規約に合わせた設定をプロジェクト内で共有しておけば、メンバー全員が同じフォーマットになります。
個人差が出にくくなるため、メンテもしやすくなります。
個人的にプロジェクト単位でガッチガチにフォーマットが設定されているほうが好きです。
たとえそのフォーマットが自分の好みではなくても。
ファイルごとにばらばらなフォーマットのコードのメンテや改修させられるよりは全然いい。
目が慣れてくると調査や読むのにかかる時間が減ります。だってフォーマット揃ってるんだから。
コメントアウトで残したままの処理は悪
バージョン管理入れてないような古いプロジェクトでありがちなんですがね。
改修前のコードをコメントアウトして残す文化は、害悪でしかないのでやめましょう。
そんなプロジェクトはとっととバージョン管理入れてください。
ゴミは残さない
コメントアウトされたコードとアンコメントされたコードが入り組んでいると、可読性が著しく落ちます。
入り組みすぎて、いざ「じゃあ○○の時にコメントアウトした部分をアンコメントして」みたいな改修で、どこからどこまでをアンコメントすればいいのかわからなくなったことがあります。
今動いてないコードは残さない。残すならバージョン管理の履歴に残して。差分が確認できるから。
何のためのバージョン管理なんだよ…
宣言とか代入での桁揃えは悪
コーディングスタイルとかフォーマットって宗教みたいなものなので、統一してほしいくらいしか言わないでおこうと思ったんですけど、これだけは言わせてほしい。
メンテや改修を念頭に置いてない、ダメダメなコードだと思ってます。
例えば、具体的に書くとこんなやつ。
String name = null;
String zip = null;
String address = null;
String telephone = null;
int sex = 0;
「emailAddress」という変数をStringで追加しようとした場合、既存のフォーマットに合わせてスペースで桁揃えしようとすると、追加する「emailAddress」の変数名が一番長くなるので、他の行も揃えなきゃならない。
もしくは、「sex」の型がintからBigDecimalに変更になった場合、Stringより型名が長いので他の行の型名のあとにスペース入れて桁揃えしなきゃならない。
いや「しなきゃならない」ではないかもしれないけど、統一されてないフォーマットはよくはないですよね。
桁揃えが好きな人って、どうしてわざわざ手間をかけてまで迷惑を意図的に仕込むの？
桁揃えしなければ、String emailAddress = null;って1行追加で済むし、BigDecimal sex = null;って型名と初期値を変更するだけで済みます。
grep検索するときもスペースの数が可変であることを考慮して正規表現で書かなくて済むし、桁揃えの利点が見つからない。
可読性が上がる？もっと別なところで可読性上げましょうよ。
メンテされるうちに絶対にどこかで桁揃えしない人が出てきます。桁揃えしないとコンパイルエラーが発生しない限り、強制はできません。
それに書いた人は等幅フォントのエディタで書いたかもしれないけど、他の人はプロポーショナルフォントで表示してるかもしれない。
例えばExcelにコード貼り付けて改修箇所の資料作らされることが結構ありますけど、デフォルトのMS P ゴシックって等幅じゃないんですよね。桁揃えの意味がないどころか不規則に並ぶので、逆に見づらいです。
表示フォントまで指定するコードを書く方がおかしいんですよ。リッチテキストでもないのに書式を指定するな。
揃えるのは論理構造を表すインデントだけでいいです。(インデント＝行頭の字下げです。行中は含みません)
※インデントのタブOrスペース論争でもよく出るけど、行中の桁揃えはインデントとは呼びません。
まあコーディング規約で指定されていれば、血反吐吐きながら桁揃えしますけど。コーディング規約は絶対。
ただ、個人的に好きじゃないです。
せめてプロジェクト内共通のフォーマッタの設定ファイルに記述して。設定ファイルすら用意しないプロジェクトでは強要しないでくれ。
おまけ1(主にif文のコメントについて)
個人的に好みの話なんですが。if文のコメントについて。
if-elseとかelseの前の項の閉じ括弧のあとで改行してコメント入れるの、やめてほしいです。JavaはC#ではありません。
下記嫌いな例その2のコードの改修で、過去に実際にバグを作りこんだことがあります。障害票起票させられました。
最早トラウマです。見かけたら全力で駆除したいです。
どこまでが1つのif文なのか判別つきにくいの、本当に嫌。そもそもそのコメントが必要か？っていうね。
あと条件分岐の数が多いなら列挙型にしてswitch文で書くとか、検討した方がいいと思う。
今のJavaならintじゃなくてもStringでもswitch文で書けたはずだし…
まあ、コーディング規約がそうなってる場合は、従うしかないんですけどね。。。
// 好ましい例
if (条件A) {
// 条件Aの場合
} else if (条件B) {
// 条件Bの場合
} else {
// 上記以外の場合
}
// 嫌いな例その1
// 条件Aの場合
if (条件A) {
}
// 条件Bの場合
else if (条件B) {
}
// 上記以外の場合
else {
}
// 嫌いな例その2
// 条件Aの場合
if (条件A) {
// 条件Bの場合
} else if (条件B) {
// 上記以外の場合
} else {
}
おまけ2(参考文献や読み物)
参考にしたい記事や面白い記事、興味深い記事など。暇なときにどうぞ。
■あっと驚かせるJavaプログラミング(をやめよう）
すげー笑った。新人さんのコードレビューでたまによく見かける事例が多いです。
■プログラマーの三大美徳
私は怠惰で短気なので、単純作業はしたくないし自分の目視が一番信用できません。
なのでPCにやらせるために、Excel関数とVBAと正規表現とSQLを覚えました。現場レベルで重用されます。
楽したい気持ち、めっちゃ重要です。
■不思議の国のSE用語
めっちゃ好きな記事。これね、なんでだろうね。気付いたらこれで会話してる。