Javaソースコードへのよくある指摘　その1

僕の仕事の一環で他人のJavaソースを見ることがあるので、その時によく出る指摘をちょっと書き出してみた。

とーってもレベルの低い記事なのでJavaをまともにやっている皆さんは読む必要無しです。ただの愚痴エントリだよ。

不要な代入
同じ式
思考停止の StringBuilder
tryでcatchしなきゃいけないものだと思っている
final配列を定数だと思っている
nullありのObjectの比較
番外編・メソッドを上手にEclipseで抽出する方法
総評

不要な代入

String name = null; // ここの代入は不要
if (conditionA) {
  name = "foo";
} else {
  name = "bar"
}

これまじやめて。

C言語(C89)から来た人はローカル変数を一番上に宣言したり、初期値を代入しなきゃいけないものだと思ってて困る。またそういうC言語特有のローカルルールをJavaの初心者に教え込んで嘘を広めていて困る。（もうC言語から来る人はそうそう出てこないと思うが。）

だいたい、nameに代入しわすれるバグも引き起こすのでやめてほしい。

if分岐の都合上、ありえないけどnameに代入されない分岐パスが出てくるなら、例外でも投げればいい。まさしく例外だから遠慮せずに投げればいい。

String name;
if (conditionA) {
  name = "foo";
} else if (conditionB) {
  name = "bar"
} else {
  throw new AssertionError("Unexpected condition!");
}

System.out.println(name);

ループで初期代入されるって？上手なかわし方を考えるか、諦めてください。

同じ式

String feces = kuso.getUnko().getShit().get("poo");
String urine = kuso.getUnko().getShit().get("piss");

こういうの本当に多い。似た式は変数かメソッドにまとめてください。そういう作業がいちいち面倒ならIDEの使い方覚えて下さい。

思考停止の StringBuilder

// ファッ！？
StringBuilder sb = new StringBuilder();
sb.append("The specified value = [");
sb.append(hoge);
sb.append("] is illegal.");
String result = sb.toString();

// これでいい。(あるいは String.format("[%s]", hoge) 使いましょう。 )
String result = "The specified value [" + hoge + "] is illegal."

見づらいのでやめていただきたい。

きっとどこかでStringの結合演算子(+)使うなっていうコーディングルール見て、その使うな、だけが脳みそに残ったか、単純にそのコーディングルールがイカれていたかなんでしょう。

ちなみにStringBuilderの多くの使いどころはループ処理で文字列結合する場合だと思いますが、Java8だと以下のようにListからStringに変換できたりするので、使う機会は減ると思います。

// Java7まで
StringBuilder sb = new StringBuilder();
for(Elem e : list){
  if(sb.length() > 0) sb.append(", ");
  sb.append(e.getVal());
}
String result = sb.toString();

// Java8から
String result = list.stream().map(Elem::getVal).collect(Collectors.joining(", "));

tryでcatchしなきゃいけないものだと思っている

try文にcatchは必須ではありません。 try {} finally {} だけでもいいのです。

// （そもそもtry-with-resources使えよっていうツッコミは無しで）

Connection c = null;
try {
  c = acquireConnection();
  transactionalProcessing(c);
} catch (HogeException e) {
  throw e; // 何もしないのにcatchして例外をもっかい投げるのは無駄
} finally {
  closeQuietly(c);
}

final配列を定数だと思っている

finalつけりゃいいってもんじゃないんだよ。時代はイミュータブルなんだよ。

// これ定数じゃない！ CONSTANT_LIST[0] = "Unko" で更新できてしまう。
public static final String[] CONSTANT_LIST = {"One", "Two", "Three", "Four"};

// これが定数！
public static final List<String> CONSTANT_LIST = Collections.unmodifiableList(Arrays.asList("One", "Two", "Three", "Four"));

Collections.unmodifiableList ・ Arrays.asList の組み合わせはよく使うので、どっかにそれ用のユーティリティ用意しておくのが普通です。

nullありのObjectの比較

java.util.Objects クラス使いましょう。

// String lhs;
// String rhs;

if (Objects.equals(lhs, rhs)) {
  // lhs == null && rhs == null の場合もOK
}

番外編・メソッドを上手にEclipseで抽出する方法

メソッドのまとめ方ですが、Eclipseならこういうふうにできます。あんまりこういうやり方を解説している人が少ないので、一応解説しておきます。

以下のソースを見て下さい。

// Map<String, String> barMap, fooMap, bazMap;

if ( Objects.equals(barMap.get("hoge"), fooMap.get("hoge")) ) {
  bazMap.put("hoge", retrieveValue("hoge"));
}

if ( Objects.equals(barMap.get("fuga"), fooMap.get("fuga")) ) {
  bazMap.put("fuga", retrieveValue("fuga"));
}

これがこうなったらわかりやすそうです。

private void setRetrievedValueIfEq(String key, Map<String, String> barMap, Map<String, String> fooMap,  Map<String, String> bazMap) { ... }

setRetrievedValueIfEq("hoge", barMap, fooMap, bazMap);
setRetrievedValueIfEq("fuga", barMap, fooMap, bazMap);

上記のようなコードにさっさとEclipseで変更するには、まずは下記のように変数を抽出(Alt-Shift-l)し、

// Map<String, String> barMap, fooMap, bazMap;

String key = "hoge";
if ( Objects.equals(barMap.get(key), fooMap.get(key)) ) {
  bazMap.put(key, retrieveValue(key));
}

String key2 = "fuga";
if ( Objects.equals(barMap.get(key2), fooMap.get(key2)) ) {
  bazMap.set(key2, retrieveValue(key2));
}

if部分を選択してメソッドを抽出すれば(Alt-Shift-m)下記のようになります。同じ構造しているものもメソッドに抽出されます。

要するにパラメータにしたいものをローカル変数として抽出してIDEが見つけられるようにしておくのがメソッド抽出のミソなんですね。

private void setRetrievedValueIfEq(String key, Map<String, String> barMap, Map<String, String> fooMap,  Map<String, String> bazMap) { ... }

String key = "hoge";
setRetrievedValueIfEq(key, barMap, fooMap, bazMap);
String key2 = "fuga";
setRetrievedValueIfEq(key2, barMap, fooMap, bazMap);

それぞれのパラメータになった変数は不要なのでインライン化(Alt-Shift-i)しましょう。

setRetrievedValueIfEq("hoge", barMap, fooMap, bazMap);
setRetrievedValueIfEq("fuga", barMap, fooMap, bazMap);

これらの操作をもうちょっとショートカットを駆使しながらほぼ無意識でできるようになるとJavaに対する見方がかわるでしょう。

（識別子をたぐる言語に見えてくるはず。）

総評

大概、知識が手習いで学んだJava1.4やってた頃でとまってる。

今日のところはこれぐらいにしておいてやるよ！