リフレクションの乱用
リフレクションや、リフレクションを使った技術(例えば、DI)の乱用はソースコードの解析を難しくさせます。
DIはクラス間の結合度を弱めて、開発やテストを効率化する効果があります。
しかし、それ故に依存関係を調べるときに苦労を伴います。EclipseなどのIDEで呼び出し階層を調べても、DI(リフレクション)による呼び出しは検出することができません。バグを修正して、その修正が他の機能に影響を与えないか確認するには、全ての呼び出し元を確認する必要があります。DIの利用箇所が明示的に分かるような実装であれば問題はありませんが、そうでない場合は影響範囲の特定が困難になります。
また、リフレクションを使えば、privateメソッドにもアクセスできます。privateメソッドのロジック修正に伴う影響範囲の確認のために、リフレクションによる呼び出しまで考慮するプログラマーはいないと思います。そんなトリッキーなコードがあるはずがないと考えて、膨大な時間を必要とする調査はしないでしょう。トリッキーなコードを書くことはリスクを産み出すことにつながります。
アノテーションの乱用
アノテーションは新しい文法の導入とほぼ同じことになり、学習コストを伴います。生産性の向上には有効かもしれませんが、保守性の観点で見ると、いいことばかりではないように思います。
次のコードはLombokというライブラリを使用してアノテーションを付加すると、
public class Person {
private String firstName;
private String familyName;
public Person(String firstName, String lastName){
this.firstName = firstName;
this.lastName = lastName;
}
public String getFirstName() {
return firstName;
}
public void setFirstName(String firstName) {
this.firstName = firstName;
}
public String getFamilyName() {
return familyName;
}
public void setFamilyName(String familyName) {
this.familyName = familyName;
}
@Override
public String toString() {
return "Person(firstName=" + firstName + ", familyName=" + familyName + ")";
}
}
このように簡略化して書くことができます。
import lombok.AllArgsConstructor;
import lombok.Data;
import lombok.ToString;
@ToString
@AllArgsConstructor
@Data
public class Person
{
private String lastName;
private String firstName;
}
ソースコード上はgetterやsetter、toStringメソッドはありませんが、実際には存在します。確かに見やすくなったし、つまらないコピーペーストのミスによるバグなども作り込みづらくなると思います。大規模な開発でこういったオブジェクトが大量に必要な場合であれば、Lombokは開発生産性を向上するソリューションになると思います。
しかし、後者はLombokの知識が無いプログラマーには直ぐには理解できません。それに対して前者のコードを読めないJavaプログラマーはいません。
さらに、アノテーションが担う処理はデバッグすることを難しくするケースも多々あります。getFirstName()にブレークポイントを張りたい場合、後者でどのようにすればいいか分からない人も少なくないと思います。IDEによってはdelombokを使わなければ、toString()だけをデバッグモードで止めることやtoString()の実装を確認することもできません。
最近のフレームワークやライブラリは独自のアノテーションを追加していることが多いため、それらを組み合わせて使うと、「アノテーション地獄」に苦しみます。
@PreAuthorize("#user.id == #u?.id")
public UserDTO access(@P("user") @Current UserEntity requestUser,
@P("u") @PathVariable("user-id") UserEntity user)
@PreAuthorize("#user.id == #uid && (#order == null || #order?.user?.id == #uid)")
public Message access(@Current @P("user") UserEntity user,
@PathVariable("user-id") @P("uid") Long uid,
@PathVariable("order-id") @P("order") OrderEntity order)
安易にアノテーションを自作することもいいことではないと思います。
その他の「黒魔術」の乱用
リフレクションやアノテーションだけでなく、トリッキーな技術はたくさんあります。その中でも特に「黒魔術」と呼ばれるような技術の利用は可能な限り避けた方がいいです。
例えば、sun.misc.Unsafeを使用すれば、static finalなクラスフィールドを書き換えるようなことができます。Javassistを使用すれば、コンパイル済みのクラスのバイトコードを直接書き換えて、実行時に動作を変更できます。黒魔術は他に実現する手段が他に無い場合や、ある程度の規模がある場合に有効な手段になりえますが、むやみに利用してはいけません。
キャッシュの乱用
キャッシュは、処理時間の短縮などに役立つため、ソフトウェアの様々な個所で利用されています。しかし、解析時には厄介な存在にもなります。キャッシュの状態によって挙動が異なる場合があるため、解析時にまるで法則性が無い動作をしているように見えることがあります。例えば、データの更新が即座に画面に反映されなかったり、DBアクセスエラーの発生有無が変わったりします。
メモリリークなどの複雑なバグを作り込む要因にもなりえるため、導入の際はキャッシュを無効化するオプションなども検討した方がいいかもしれません。
技術の乱用
プログラマーがJavaを習得していれば、Javaのアプリケーションを実装できます。さらにServlet/JSPを習得していれば、Webアプリケーションを構築できます。Spring Bootを習得していれば、開発の生産性を上げることができます。ThymeleafやBootstrapを習得していれば、デザインとロジックを分離し、デザインのクオリティや可読性の向上ができます。SAML 2.0とSprig Securityを習得していれば、認証を外部システムに委譲することができます。JUnitやGradleを習得していれば、テストやビルドを自動化できます。他にも習得することで、実現できることはたくさんあります。
しかし、それらが全てできるプログラマーは限られています。人によって読みやすさの基準はさまざまですが、扱う技術が増えれば増えるほど、そのすべてのコードをすらすら読めるプログラマーは減ります。扱う技術を闇雲に増やすと、総合的に可読性は下がることになります。より簡潔に書けるという理由で、別の言語や技術で部分的に実装を書き換えても、それを扱えるプログラマーが少なくなれば、結果的に可読性は下がり、解析にかかる時間が増えます。
嘘つきなコメント、Javadoc、ログ
コメントやJavadocはソースコードほど考えて書かない人が多いと思います(私もそうですが...)。
以下のような記載があるにもかかわらず、実際はバージョン3.0.0から導入されたクラスがあると、調査の前提条件が崩れることになり、混乱が生じます。
/**
* ...
* @since 2.0.1
*/
正しくないコメント、Javadocを書くくらいなら何も書かない方がましです。バージョン管理システムで履歴を見れば分かるので、@sinceのような正しくない二重管理の可能性があるものは使わない方が無難かもしれません。
他にも嘘をつくり込む危険性が高いものはあります。フィールド変数のLoggerのコピーペーストにより、以下のようなクラス名と実クラスが異なるコードは実装してしまいがちです。
public class DeleteController {
private static final Logger log = LoggerFactory.getLogger(ImportController.class);
このロガーが出力したログは混乱のもとになります。本番環境にリリースする前にソースコード全体をgrepして確認した方がいいかもしれません。
キーとなる値の分割
調査の際に困るのが、propertiesファイルに定義したプロパティキーの一部を定数などに分割してしまう実装です。例えば、以下のようなプロパティが定義されていたとします。
account.lockout.count=10
account.lockout.interval=60000
これを取得するために次のようなコードを書いたとすると、
private static final String ACCOUNT_LOCK_PREFIX = "account.lockout.";
...
private void doSomething() {
ResourceBundle rb = ResourceBundle.getBundle("application");
String count = rb.getString(ACCOUNT_LOCK_PREFIX + "count");
String interval = rb.getString(ACCOUNT_LOCK_PREFIX + "interval");
プロパティキーの「account.lockout.count」で全てのソースコードをgrepしてもヒットしなくなってしまいます。その結果をもとに、このプロパティは使われてないと判断して、propertiesファイルからこのキーを削除したら、実行時にMissingResourceExceptionがスローされてしまいます。キーとなるような文字列は分割しないことをお勧めします。
役に立たないログ
エラーログのメッセージなどもプロパティキー同様に考慮が必要です。ソースコードをgrepして使用個所を特定できるようなメッセージであることが望ましいです。同一のログメッセージが複数あると、それを出力した個所が特定できません。ログライブラリの出力形式設定で、ソースコードのファイル名と行数をログに出力するようにしていればいいですが、性能を考慮して(またはデフォルトを見直していないため)、そうなっていないことも多々あります。
例えば、以下のようなログ出力処理は
log.warn("{} not found", data.getId());
可能であれば、出力した個所を特定しやすいように、以下の2つに分割した方がいいでしょう。
log.warn("User does not exist in user table: {}", user.getId());
と
log.warn("Group does not exist in group table: {}", group.getId());
他にもログ出力について検討すべきことは多いです。例外をcatchして、その例外を利用していない実装は、問題を隠蔽することになります。
} catch (Exception e) {
}
調査に役立つようなログを適切なログレベルと適度なタイミングで出力していると、ソースコードの解析が楽になります。
さらにWebアプリケーションの場合、再起動せずにログレベルを変更できる機能があると、非常に便利です。私が仕事で扱っているOpenAMというソフトウェアにはこの機能があり、管理者向けのWebアプリケーションでログレベルを変更ができます。本番環境で問題が発生した場合でも、一時的にログレベルを上げてから事象の再現させて、ログを取得できます。取得し終わったら、ログレベルを元に戻せばいいわけです。
OpenAMにはトラブルシューティング用のREST APIもあります。特定のURLにリクエストを送信すると、システムの設定ファイル、一定時間のデバッグログとスレッドダンプなどを、まとめてzipファイルに圧縮し、出力してくれます。このように解析を目的とした機能をつくっておくと、運用時には大きな助けになります。
設定が動作を決定するような仕組み
フレームワークを開発するような場合、XMLファイルやJava Configなどの設定が動作を決定するような仕組みをつくると、その動作を追いづらくなることが多いです。
最近(よくない意味で)ハマったのがSpring Securityのアクセス制御(認証、認可、セッション管理、CSRF対策など)の設定です。以下のようなコードを書くと、その内容にもとづいてアクセス制御してくれるという機能なのですが、少し複雑なことを実現させようとすると、なかなか思い通りに行きません。
@Override
protected void configure(HttpSecurity http) throws Exception {
http.antMatcher("/admins/**").authorizeRequests().antMatchers("/admins/login").permitAll()
.antMatchers("/admins/**").authenticated().and().formLogin().loginProcessingUrl("/admins/login")
.loginPage("/admins/login").failureUrl("/admins/login?error").defaultSuccessUrl("/admins/main")
.usernameParameter("username").passwordParameter("password").and().logout().logoutUrl("/logout")
.logoutSuccessUrl("/").deleteCookies("JSESSIONID").and().sessionManagement().sessionFixation()
.none().and().csrf();
}
「少し複雑」とは言っても、サーブレットフィルターにロジックを追加すれば、簡単に実装できるレベルだったのですが、そのロジックをSpring Securityのルールに落とし込もうとすると、どのような設定をして、どのようなクラス、メソッドを実装すべきかなかなか判断が付きません。
さらに自分が書いたサーブレットフィルターをデバッグすれば、どこに問題があるかは直ぐに特定できますが、Spring Securityの汎用的なロジックをデバッグするのは簡単ではありません。上のconfigure()メソッドのように実行時にデバッグできないような実装、設定を要求すると解析を難しくします。
設定だけで複雑なことを実現できる仕組みを新たにつくった場合、いろいろなパターンを想定して、なぜ動かないかを開発者に気付かせるようにしなければなりません。また、その利用方法に対する開発者の学習コストも上がることも十分に考慮すべきです。
過度な短さの追求
リーダブルコードにも書いてありましたが、ソースコードは短くすれば読みやすいというわけではありません。次のようなコードゴルフで競うソースコードが決して読みやすいものではないことを考えれば当然のことです。
class G{static public void main(String[]b){for(int i=0;i<100;System.out.println((++i%3<1?"Fizz":"")+(i%5<1?"Buzz":i%3<1?"":i)));}}
これではブレークポイントをセットすることもできません。テクニックを駆使して短くするくらいなら、冗長なコードを書いた方がいいと思います。「短いコード＝読みやすいコード」という固定観念が、解析の難しいコードを生み出す要因になっているような気さえします。
新しい文法
Java 8でラムダ式が追加されましたが、これもデバッグがしづらいケースが多々あります。例えば、以下のようなfor文は
for (String n : notifications) {
System.out.println(n);
}
Java8では次のように書くこともできます。
descriptions.forEach(n -> System.out.println(n));
このfor文において、文字列nの長さが100を超える時だけループを止めたい場合、次のような条件付きのブレークポイントをセットします。
前者のSystem.out.println(n);の行に条件付きのブレークポイントをセットした場合は、その条件の時にプログラムが一時停止しますが、後者にブレークポイントをセットした場合は、次のようなエラーが出てしまいます(※STS 3.8.4を使用しています)。
IDEが最新の文法に全て対応していればいいのですが、必ずしもそうはなっていません。1行のラムダ式を3行に分けて書かないとデバッグができないといったこともあります。新しい文法で実装した場合は、一度ブレークポイントをセットして動きを確認してみる必要があるかもしれません。
メソッド連鎖
最近のソースコードで良く見かけるperson.setName("Peter").setAge(21)...のようなメソッド連鎖(Method Chaining)ですが、これも解析においてはいい書き方とは言い難いです。
class Person {
private String name;
private int age;
// In addition to having the side-effect of setting the attributes in question,
// the setters return "this" (the current Person object) to allow for further chained method calls.
public Person setName(String name) {
this.name = name;
return this;
}
public Person setAge(int age) {
this.age = age;
return this;
}
public void introduce() {
System.out.println("Hello, my name is " + name + " and I am " + age + " years old.");
}
// Usage:
public static void main(String[] args) {
Person person = new Person();
// Output: Hello, my name is Peter and I am 21 years old.
person.setName("Peter").setAge(21).introduce();
}
}
setterが自身のインスタンスを返すというのは、オブジェクト指向の観点からも直感的ではないように感じます。メソッド連鎖の可読性は人によって異なるかもしれませんが、デバッグのしやすさを考えると、あまりいい書き方とは言い難いです。この行では、
person.setName("Peter").setAge(21).introduce();
ブレークポイントを適切なポイントにセットすることができないので、必要な場所でプログラムを一時停止できません。また、これらのメソッドの1つが例外をスローした場合、行番号は全て同じになるのでどのメソッドが問題を引き起こしたか分かりません。
Exception in thread "main" java.lang.NullPointerException
at tdd.Person.main(Person.java:27)
これを以下のように分割していれば、
person.setName("Peter"); // 27行目
person.setAge(21); // 28行目
person.introduce(); // 29行目
スタックトレースを見ただけでどのメソッドが問題であるかが分かります。
Exception in thread "main" java.lang.NullPointerException
at tdd.Person.main(Person.java:29)
デバッグしにくい、修正を反映させにくい仕組み
数年前に開発していたWebアプリケーションでは、Excelの画面仕様書を入力としてツールが出力したXMLファイルをフレームワークが解析し、画面を生成するような仕組みとなっていました。
この方法で画面のデザインの統一と均質化は図れるのですが、想定したレイアウトや動作とならない場合の解析は非常に面倒なものでした。
当然ですが、Excelの画面仕様書もXMLファイルもデバッグできません。ブレークポイントをセットできるのはそのXMLを解析するフレームワークのソースコードになります。フレームワークのソースコードをデバッグするにしても、仕様書の変更のトライアンドエラーを繰り返すにしても時間がかかります。さらにそれだけでなく、修正してから、XML出力、デプロイ、確認するまでにも時間がかかります。Tomcatのオートリロードを有効にしていても、この作業には時間がかかりました...
最近流行りのSpring BootとThymeleafを使った開発では、画面やロジックの修正の後にコンテナの再起動をする必要がありません(全く必要ないわけではないですが)。ThymeleafはテンプレートとなるHTMLファイルをキャッシュしなければ、即座に修正を反映させることができます。JRebelやSpring Loadedなどを使えば、Javaのソースコードの修正も即時反映して確認ができます。
デバッグしにくい、修正を反映させにくい仕組みをつくってしまうと、その後の作業で大きなタイムロスになります。
時間のかかる起動処理
Tomcatなどのコンテナで動作するWebアプリケーションの初期化処理は、起動時に行われることが多いと思います。例えば、DBコネクションプールの生成などがこのタイミングで行われますが、必ずしも起動時に行わなければならないわけではありません。初回DBアクセス時や起動後にバックグラウンドで行っても、必ずしも遅いわけではありません。開発プロセスだけでなく運用やトラブルシューティングにおいても、コンテナの再起動は何回もすることになります。1回の起動時間が数秒増えたとしても、(増えた秒数)×(開発者の数)×(再起動の実行回数)の時間が無駄になります。
ある現場でTomcatの起動に5分かかるようなWebアプリケーションがあり、誰もそれに対して疑問を持たず、開発をしていました。これはおかしいと思い、遅延の原因を調べたところ、初期化処理時間の90%程度を一つの処理が占めており、これを改善することで起動時間は20秒程度まで改善されました。コンテナの起動時間が、Webアプリケーションをデプロイするかどうかで大幅に異なるような場合は、一度Webアプリケーションの初期化処理の内容を見直した方がいいかもしれません。
まとめ
思いつくままにいろいろ書いてしまったので、あまり整理されていませんが、解析しにくい、デバッグしにくいコードは他にもいろいろとあります。そうならないようにするには、以下の点に気をつければいいかと思います。
- 読みやすいコードを書くことの他に、デバッグしやすいか、解析しやすいかも考えてコーディングする
- デバッグできるかどうか、しやすいかどうかを実際にIDEで動かして確認してみる
- ログやコメントのメッセージが、解析に役立つかを常に留意する
- トリッキーな技術や新しいルールの導入の際は十分な検討をする
- テクニックに走らない
- 短く書くことにこだわらない
- デバッグや解析が難しい、または時間がかかる仕組みをつくらない
最後に、偉そうなことを言ってしまいましたが、そんなコードを自分が書いているかというとそうでもありません...
