僕が仕事で昔の自分や他のメンバーがコードを読んでいて「これ、もっと良い書き方できないかな」と思ってしまう書き方のまとめです。半分以上は自戒です。
（2016/10/19 追記）
タイトルが「Javaの言語仕様で好きになれない書き方」という意味だと誤解を与えそうだったため、修正しました。
- 修正前
- どうにも好きになれないJavaの書き方
- 修正後
- 【Java】どうにもコーディングし直したくなってしまう書き方まとめ
（/追記）
内容的に個人的な好みの問題も多分に含まれるとても主観的な記事ですので、「よし、気をつけよう」と思うのか「いやいや全然問題ないでしょ」と思うのかは読む方にお任せします。「そんなの当たり前でしょ」な内容もあるかもしれません。
とはいえ、どれも実際に仕事で目にしたことのあるコードで、好きになれないのも理由があってのことですので、この記事では「なぜ好きになれないのか」を重点的に書こうと思います。また、可能な範囲で書き直した例も載せてみます。
本文に入る前に、Javaのコーディングのアンチパターン集としてこちらの記事がとても参考になりましたので、先に紹介します。
こちらの記事に書かれていることはほぼそのまま同意ですので、同じことを繰り返しはせず、なるべく上記の記事には書かれていない内容を書くようにします。
ではどうぞ。
本文
メソッドの中で関係ないことまでしてる
まずは、このパターンです。
例えばこんな感じのコードです。1
public Article getArticle(String articleId) {
...何か記事を取得する処理
// 取得した記事をDBに保存
new ArticleDao().insert(article);
}
このメソッドは、シグニチャを見る限りはgetArticle()という一見引数のarticleIdに紐づく記事データを取得する__だけ__をすることが推測されます。つまり、IDというインプットと記事データというアウトプットの他には副作用はないように思えます。しかし、実際に使ってみるとなぜか呼ぶたびにDBに新たなレコードが追加され、場合によってはそれを取得する処理で不具合が発生したり、このメソッド内で意味不明なSQLExceptionが発生したりするかもしれません。
もしかしたらこのメソッドを作った人は、その時「記事をQiita APIで取得してDBにキャッシュする」までがセットの処理を書いていたのかもしれません。そして、このメソッドは__その時は__うまく動いていたのかもしれません。
しかし、後からこのメソッドの存在を知った別のメンバーは、きっと記事IDを渡せば記事データが取得できる便利なメソッドだと思うでしょう。もしくは、何かDBに異常が見つかってデバッグしなければならないとき、まさかgetメソッドの中に原因のコードがあるとは思わないでしょう。
誰か他の人（もしくは昔の自分）が書いたメソッドの一つ一つの実装をちゃんと読んで理解していくのはとても労力が必要になります。また、そもそもメソッドを使う人はコンパイル済みクラスしか手元になく、ソースコードが簡単に読めない状態にあるかもしれません。書いた人は実装の詳細を把握していますし、もしかしたら「使う前にちゃんと読んでよ」と思うのかもしれませんが、現実的にそれは難しい場合がほとんどです。
なるべく、シグネチャからは予想できない処理は書かない（もしくはjavadocに明記しておく）ことが、事故を減らす一つの改善策になるのではないかと思います。
それを踏まえると、上記のコードはこんな感じが良いでしょう。
public Article getArticle(String articleId) {
...何か記事を取得する処理
return article;
}
public void cacheArtice(Article article) {
// 取得した記事をDBに保存
new ArticleDao().insert(article);
}
これなら記事データが欲しければgetArticle()を、キャッシュしたければcacheArticle()を個別に呼び出せるので、予想外の挙動が少なくなります。別の処理は別のメソッドとして切り分け、呼ぶ側で個別に呼ぶ、という形にすることで読みやすくもなり、もし「記事を保存したいだけ」という要件が出てきたときにも対応が楽になります。
コメントが嘘
どこかの処理を参考にしようとコピペしたら、処理は直したけどコメントを直し忘れたパターンです。もしくは、一度作ったところを後から機能拡張や仕様変更した際にコメントを直し忘れた、というパターンもあります。
/**
* IDに紐づく記事を取得します。
*/
public Article getArticle(int articleId) {
...何か記事を取得する処理
}
/**
* IDに紐づく記事を取得します。
*/
public List<Comment> getComments(int articleId) {
...何かコメント一覧を取ってくる処理
}
という感じでしょうか。
getArticle()を真似してgetComments()を作ったら、コメントがgetArticle()のもののままになっています。
コードを読む時は処理の内容を理解する良いとっかかりとしてコメントを見ることが多いため、コメントに嘘つかれるとその時点で読む労力が倍になります。上記の例のようにコピー元が近ければ明らかにコピペだとわかるのですが、別のクラスからのコピペだったりすると、もはやそのコメントが嘘なのか、自分の理解が間違っているのか判断できないためです。また、上記の例ではコメントに関する処理を探そうと「コメント」で検索しても出てこなくなっていて、やはりコードが読みづらくなってしまっています。
実際仕事をしていると時間的に余裕がない時はあって、同じような処理をコピペで何とかしてしまおうとするのも気持ちはわかるのですが、最低限嘘だけは書かないように気をつけたいところです。
メソッドを呼び出す順番が決められてる
メソッドAの結果をメソッドBで使う、ではなく、メソッドAを先に呼び出してからメソッドBを呼び出さないとバグる、というものです。
例えば以下のような感じです。
/**
* IDに紐づく記事本文を取得します。
*/
public Article getArticle(int articleId) {
this.qiitaRequest = new QiitaRequest();
this.qiitaRequest.request(QiitaPath.ARTICLE);
... その他の処理
}
/**
* IDに紐づくコメント一覧を取得します。
*/
public List<Comment> getComments(int articleId) {
this.qiitaRequest.request(QiitaPath.COMMENTS);
... その他の処理
}
上記の例の場合、getArticle()を呼ぶ前にgetComment()を呼んでしまうとNullPointerExceptionで落ちます。
これも、書いた人はその時「getArticle()が先に呼ばれるように書いてるから大丈夫！」と思っているのかもしれませんが、ソースコードはその後誰か別の人（もしくはこのことを忘れた自分）が修正を加えたり、片方のメソッドだけ使うような別の実装を追加するかもしれません。その時に、この順序が実は決められている、という仕様を知らずにコーディングしてしまうことで、簡単にバグが発生します。
どの処理でも必要な事前準備をするように書いておくか、コンストラクタ等必ず呼び出される部分で必要な事前準備は済ませてしまい、どのメソッドがどの順番に呼ばれても問題ないようにしておくのが安全です。
/**
* コンストラクタ
*/
public ApiAccessor {
this.qiitaRequest = new QiitaRequest();
}
/**
* IDに紐づく記事本文を取得します。
*/
public Article getArticle(int articleId) {
this.qiitaRequest.request(QiitaPath.ARTICLE);
... その他の処理
}
/**
* IDに紐づくコメント一覧を取得します。
*/
public List<Comment> getComments(int articleId) {
this.qiitaRequest.request(QiitaPath.COMMENTS);
... その他の処理
}
たしかに、インスタンスの状態によってメソッドが失敗する場合というのは珍しくないですし、間違っているわけでもない場合があります。しかし、「そのメソッドを書いた時に与えられていた仕様ではこの順番でしか呼ばれないから」くらいの理由だけで呼び出し順を強制するのは、さすがにメソッドの分け方を間違えていると言える場合の方が多いのではないかと思います。
文字列で判定する
何らかの条件分岐を、文字列で判断する書き方です。
例えば以下のような書き方です。
// 環境によって変わる設定値を取得します。
public Config getConfig(String environment) {
if (environment.equals("develop")) {
return new DevelopConfig();
} else if (environment.equals("production")) {
return new ProductionConfig();
} else if (environment.equals("test")) {
return new TestConfig();
}
return new DevelopConfig();
}
※ ConfigクラスはDevelopConfig等の各設定クラスの親クラスとします。
僕のイメージですが、文字列はとても「心もとない」です。想定外の文字列が渡されたとしてもコンパイル時に検知することができず、実行時に何らかの不具合として表面化するまで間違いに気づくことができないからです。
例えばメソッド中の"develop", "production", "test"のどれか１文字でもタイポしたらif文が正常に機能しなくなりますし、引数として渡すenvironmentも同じくタイポや想定外の文字列が含まれたらDevelopConfigしか返却されなくなります。そして、それは実行して不具合が発生するまで気づくことができません。
バグの芽はなるべく早いうちに発見して潰してしまった方が楽です。実行時よりコンパイル時、コンパイル時よりコーディング時です。
例えばこの場合、enumを使って書くことでコーディング時に少なくともタイポ等のケアレスミスでバグが発生することはなくなります。2
public enum Environment {
DEVELOP,
PRODUCTION,
TEST;
}
// 環境によって変わる設定値を取得します。
public Config getConfig(Environment environment) {
if (environment == DEVELOP) {
return new DevelopConfig();
} else if (environment == PRODUCTION) {
return new ProductionConfig();
} else if (environment == TEST) {
return new TestConfig();
}
return new DevelopConfig();
}
引数がStringの場合、このメソッドを使う人は何を渡せば良いのか実装を見ないとわかりませんが、enumを使えばDEVELOP, PRODUCTION, TEST以外を渡すことができず、実装を見ることなく適切な引数を選択することができます。
さらに、どのifにも入らなかった場合（つまりnullが渡された場合）のreturn new DevelopConfig()というデフォルト値をenum側で決めてしまうのに違和感がある場合は、以下のように書き換えられます。
public enum Environment {
DEVELOP(new DevelopConfig()),
PRODUCTION(new ProductionConfig()),
TEST(new TestConfig());
private Config config;
Environment(Config config) {
this.config = config;
}
// 環境によって変わる設定値を取得します。
public Config getConfig() {
return this.config;
}
}
こうすれば、使う側はEnvironment.DEVELOP.getConfig();のように書くだけで、条件分岐なく目的の設定クラスを取得することができ、またEnvironmentのenum側は特にnull等の異常系を考えなくてよくなります。
まとめると、if分岐で比較する対象は文字列よりもenumが良いでしょう。
メソッド名が動詞（＋名詞）でない
基本的にメソッドは「振る舞い」を表すものです。ということは「〜をする」という動詞が何か当てはまるはずです。そして、英文法上、動作を表す語順は「動詞 + 目的語」もしくは「動詞 + 補語」です。いずれにしてもメソッド名の始まりは基本的に動詞であるのが自然です。
しかし、実際に書かれたコードを読んでいると、割と頻繁に「動詞（+名詞）」のパターンから外れているメソッド名を見かけます。
例えばこんな感じです。
public Article article(int articleId) {
...何か記事に関する処理
}
public List<Comment> comments(int articleId) {
...何かコメントに関する処理
}
これも、書いている人はその時の流れで記事をどうしているのか、コメントをどうしているのかが分かるのかもしれませんが、初めて見る人にとっては記事を「取得する」のか、「更新する」のか、「削除する」のか、それとも「出力する」のか、実装を見てみるまで全くわかりません。また他の部分でこのメソッドを使い回すなんて怖くてできません。
メソッドである以上「〜する」は必ず表せるはずですので、なんとか動詞で始まるメソッド名をつけるのが良いと思います。
一方で、toString()やsize()など、動詞でなくても慣習的に使われているメソッド名もあります。確かに最終的にメソッドの内容が伝われば動詞でなくても良いのですが、個人的にはこの命名はJavaが提供するAPI、もしくはよく設計されたライブラリやフレームワークの話だけくらいに考えていた方が良いと思います。経験上、自分でこのような命名を考えてうまく伝わる名前が出てきた試しがないためです（自分にセンスがないだけかもしれませんが）。多少冗長でも、やはり動詞で始めるのが無難でしょう。
isShow()
「is + 動詞」のパターンです。英文法上とても違和感があります。例えるなら、"He is show his car." と言っているようなものです。
このネーミングは、文法的に違和感があるだけでなく、結局どんな時にtrueになるのかわからない、という点も問題です。「今表示している（isShowing）」なのか、「表示すべき場面である（shouldShow）」なのかが曖昧で、作った人以外は実装をよく読まないと判断できません。
boolean型のメソッドだからisで始める、というのは理解できるのですが、他にもhas, should, can など、boolean型のメソッドで使えそうな単語が他にもあるのではないかと思います。
とりあえずnullチェックして回避する
NullPointerExceptionを回避するため、とりあえずnullチェックしておく、というものです。
nullチェック自体は問題ない（というかかなり重要）のですが、チェックした結果ダメだった場合に、処理をスキップするだけ、という書き方がとても違和感があります。
以下がその例です。
Article article = getArticle(articleId);
if (article != null) {
... 記事データを何かする処理
}
... articleがnullだった場合は何事もなかったかのように後ろの処理へ
例えば記事取得に失敗したのであれば、リトライする、例外を投げて呼び出し元に異常を伝える、デフォルトのデータを使う、等いろいろと選択肢があると思うのですが、とにかくNullPointerExceptionで落ちなければ良いと、nullチェックのif文で大きく囲ってしまう__だけ__の書き方がとても気になります。いっそNullPointerExceptionでクラッシュさせた方が、明らかな異常が発生していることをプログラムの実行者がすぐに検知できてまだマシなのではないかと思えてしまいます。
結局if文で回避しても、article変数はそのメソッド内ではずっと参照できてしまうため、もしかしたらその不正なデータを不正なままDBに保存したり、HTTP通信を通してサーバーに送ってしまうかもしれません。そうすると、もしDBやサーバー内で異常が起こった場合に異常が起こっている箇所とその原因が離れすぎていて特定がとても困難になる可能性が出てきます。
NullPointerExceptionで落ちないために機械的にnullチェックをするのではなく、「チェックした結果どうするのか」にもう少し労力をかけても損はないのではないかと思います。
ArrayList<String> idList = new ArrayList<>();
インターフェース（抽象クラス）で受け取ることができる（ふさわしい）状況で、生成した具象クラスのまま受け取るパターンです。特にArrayListやHashMapでよく見ます。
ListやMapのように、インターフェースさえ分かれば十分に扱えるクラスの場合、インスタンスを生成した後はインターフェースとして扱っていた方がその後の変更に良いとされています。
例えば、以下のようなコードがあったとして、
List<Article> articleList = new ArrayList<>();
... 何かarticleListを使った処理
一度実装した後にArrayListよりもLinkedListの方が処理効率が良い、ということがあった場合、上記のコードであれば修正箇所はnew ArrayList<>()をnew LinkedList<>()にするだけです。
しかし、以下のように
ArrayList<Article> articleList = new ArrayList<>();
... 何かarticleListを使った処理
cacheArticleList(articleList);
...
public void cacheArticleList(ArrayList<Article> articleList) {
... 何かの処理
}
となっていた場合、ArrayListで宣言されている場所を全て修正しなければならなくなります。さらに、上記の例のcacheArticleList()がコンパイル済みだったりすると、もう変更は不可能です。
具象クラスの種類にこだわりがないのであれば、常にインターフェースで扱うのが良いでしょう。
まとめ
こうしてまとめてみると、読んでいて気になるのは「他の人が読みやすいかどうか、誤解なく読めるかどうか」がほとんどだったりします。逆に、処理効率や高速化はそれほど気になりません（というかあまり詳しくないのでツッコめません）。
あまり複雑でない、またリクエストも膨大な訳ではないWebアプリケーションを扱っている限り、おそらく優先されるべきはユーザーが実感できない高速化よりも開発効率を上げるソースコードの読みやすさになることも少なくありません。
この記事を書きながら、同時にEffective Javaを読んでいるのですが、やはり書いてあることは同じだったりします。（特に「プログラミング一般」の章の項目のいくつかはこの記事を書く上でだいぶ参考にさせていただいています）
もう一つ、（翻訳）Optionalは最も優れた型であるの原文の筆者も繰り返していましたが、ミスはなるべくコンパイル時に見つかるような書き方が望ましい、というのも改めて感じた部分でした。文字列ではなくenumを使う、というのはその最たる例かと思います。人はどうしてもミスをしてしまいますので、ルールや説明でなんとかするには限界があるため、「そもそも仕組み上ミスできない」という状況はとても強いです。
期限がすぐそこに迫っていて、ちゃんと考えて書いている余裕がないことは往々にしてあるかと思いますが、それでもこの記事のいくつかの項目はものの数秒で改善できる部分です。少なくともそのような「ちょっとやれば結構良くなる」系の部分だけでも気をつけ、自然にできるようになれれば、チームメンバー、未来の自分、未来の保守担当みんながシアワセになれるのではないかと思います。
書きたかったことは他にもある気がしていますので、思い出したら随時更新します。