※本記事は、Ten Java coding antipatterns to avoid: Worst practices #5 through #1 の翻訳記事です。
2022年7月30日| 9分読む
これらの最悪の慣習を回避し、既存のコードを保守またはリファクタリングするときに修正する必要があります
他のユーザーが書いたコードや自身が書いたコードをみて、驚き、不信心感、嫌悪感で時々頭を叩きませんか。。
前の記事「回避すべき10のJavaコーディング・アンチパターン: 最悪の習慣#10から#6」では、5つのアンチパターンを探りました。残りの5つの最悪の習慣とボーナスで議論を終えます。
前の記事の紹介で書いたことを繰り返します。これらの最悪の習慣を回避し、既存のコードを保守またはリファクタリングするときにそれらを排除すべきです。もちろん、コード・レビュー中にこれらの問題を見つけたら解決します。
最悪の習慣 #5: コードの複製
多くの開発者は、コピー・アンド・ペーストが悪い考えであることを教えられます。アプリケーションの他の場所からコードをコピーすると保守の悪夢が生じるため、文字通りに悪くなります。バグを見つけたり機能を変更したりするには、すべてのコピーを見つけてすべてを修正する必要があります。コピーはまたプログラムを不必要に大きくする欠点もあります。
多くのIDEには、既存のコードを取得し、新しいJavaメソッドに変換する”extract method”または”introduce method”リファクタリング関数があります。コピー・アンド・ペーストではなくメソッドを作成すると、コードがより短く、より明確になり、デバッグとメンテナンスが容易になります。PMDオープン・ソース・プロジェクトからのコピー・アンド・ペースト・ディテクタであるCPDは、コピー・アンド・ペーストが適用された場所を見つけるための便利なツールです。賢いアルゴリズムを使用して重複トークンを検出し、デフォルトでは100個以上のトークンの実行を検索します。そのほとんどは、コピーを宣言するために同一である必要があります。トークンは、キーワード、リテラル、演算子、セパレータ、識別子などの要素です。
CPDは、拡張可能な言語間静的コード・アナライザであるPMDの一部として配布されます。
オープン・ソースのGitHubリポジトリの1つには、Java Cookbookのすべてのコード例と、他の多くのコード・サンプルが含まれています。残念ながら、本書で使われていない例のいくつかは、必要な定期メンテナンスをされていません。
(実際のアプリケーションの作成には適用されないということを正当な理由として、開発者がコード例をコピーすることがあります。)
この記事を書いている間、リポジトリに対してCPDを実行しましたが、いくつかの問題が見つかりました。ここに2つあります。
$ cpd
Found a 14 line (184 tokens) duplication in the following files:
Starting at line 19 of /home/ian/git/javasrc/desktop/src/main/java/gui/FlowLayoutSimple.java
Starting at line 37 of /home/ian/git/javasrc/desktop/src/main/java/gui/FlowLayoutSimple.java
getContentPane().add(quitButton = new JButton("Stop"));
getContentPane().add(quitButton = new JButton("Exit"));
getContentPane().add(quitButton = new JButton("Exit"));
getContentPane().add(quitButton = new JButton("Exit"));
getContentPane().add(quitButton = new JButton("Exit"));
getContentPane().add(quitButton = new JButton("Exit"));
getContentPane().add(quitButton = new JButton("Exit"));
getContentPane().add(quitButton = new JButton("Exit"));
getContentPane().add(quitButton = new JButton("Exit"));
getContentPane().add(quitButton = new JButton("Exit"));
getContentPane().add(quitButton = new JButton("Exit"));
getContentPane().add(quitButton = new JButton("Exit"));
getContentPane().add(quitButton = new JButton("Exit"));
getContentPane().add(quitButton = new JButton("Exit"));
１番目はおもしろいですね。これは明らかに編集エラーです。viエディタを使用すると、数値の後に挿入を行うと、その挿入のコピーの数が挿入されます。ただし、数字の後に文字G (for go)が続く数字は、数字で1行にジャンプするために使用されます。
数字を入力して行にジャンプし、Gを忘れて、その場所に挿入する行を入力したため、行は何度も誤って挿入された可能性があります。不思議なことに、この誤りは2003年から私の公開リポジトリにありますが、誰もそれを私に指摘してきたことはありません。
2番目の問題は、次のファイルで18行(184トークン)の重複を識別しました。
Starting at line 28 of /home/ian/git/javasrc/main/src/main/java/regex/LogRegEx.java
Starting at line 25 of /home/ian/git/javasrc/main/src/main/java/regex/LogRegExp.java
System.out.println("Input line:" + logEntryLine);
Matcher matcher = p.matcher(logEntryLine);
if (!matcher.matches() ||
LogParseInfo.MIN_FIELDS > matcher.groupCount()) {
System.err.println("Bad log entry (or problem with regex):");
System.err.println(logEntryLine);
return;
}
System.out.println("IP Address: " + matcher.group(1));
System.out.println("UserName: " + matcher.group(3));
System.out.println("Date/Time: " + matcher.group(4));
System.out.println("Request: " + matcher.group(5));
System.out.println("Response: " + matcher.group(6));
System.out.println("Bytes Sent: " + matcher.group(7));
if (!matcher.group(8).equals("-"))
System.out.println("Referer: " + matcher.group(8));
System.out.println("User-Agent: " + matcher.group(9));
});
同じプログラムが、一般的な Apacheログファイル形式を解析するための正規表現の使用を実証したため、2つの異なる名前を持つ同じファイルを誤って作成したように思われます。
私がよくお勧めするツールに、ダメ出しをされた訳です。もっと頻繁にCPDを使わなければなりませんね。
最悪の習慣 #4: 古いJavadoc
Javadocはあなたの友人ですが、友達でいるには努力が必要です。ドキュメントを読み取って有用に適用するには、最新である必要があります。そのため、引数を関数に変更する場合、たとえばJavadocを変更する必要があります。次のことを担当する開発者ではありません。
/**
* Perform the aFunction function.
* @parameter x The X coordinate to start
* @parameter y The Y coordinate to start
* @Parameter len The number of points to process
*/
public double aFunction(double x, double y, double endX, double endY) {
Javadocは、参照するためにHTMLなどの形式で生成されるのでより便利です。MavenおよびGradleおよびその他のビルド・ツールには、ビルド・プロセス全体の一部としてJavadoc Webページを簡単に生成できるプラグインがあります。Mavenでは、Javadocを操作するために10行または15行のプラグイン構成が必要になる場合がありますが、これが記述されると、その構成はほとんど変更されません。
開始時に次の構成要素を含めることができます。
<failOnError>false</failOnError>
ところで、古くて散発的に維持されているJavadocは、ビルドが完全に失敗することがあります。これは、ドキュメントを段階的にクリーンアップし、他の開発者に表示することを誇りに思う状態にするよい機会です。
最悪の習慣 #3: 未検証のユーザー入力
1979年、Brian KernighanとP.J. PlaugerはThe Elements of Programming Styleという本を書きました。KernighanとPlaugerの本は、例がいくつかの古いプログラミング言語で書かれていますが、真に時代を超えたな開発者からのアドバイスが含まれています。私の好きな名言は
ユーザー入力を信頼しないでください。
正確には「入力の信憑性と妥当性をテストせよ」と書いてあるのですが、私はこの表現の方が好きです。
JDBCコールを記述したコードを読み取る場合、最初の文でこのアンチパターンを見つけることは珍しくありません。
rs = jdbcConnection.createStatement().executeQuery( // 悪い
"select * from customers where name = '" + nameField.getText() + "';");
PreparedStatement statement = jdbcConnection.prepareStatement(
"select * from customers where name = ?1"); // 良い
statement.setString(1, nameField.getText());
rs = statement.executeQuery();
nameField.getText() の値は、ほぼ確実にユーザーから直接取得されます。つまり、信頼できないユーザー入力です。ただし、このデータはデータベースに直接送信されます。
“327: Exploits of a mom” に示すように、悪い人が次のように入力するとどうなるでしょうか。
John Smith'; drop table customers; --
次のSQLを入力したかのようになります。
"select * from customers where name = 'John Smith'; drop table customers; --';"
多くのJDBCドライバでは、1行に複数の文を使用でき、それぞれにセミコロン(;)を付けます。データベース・アーキテクトが開発者と同じくらい不注意だったらどうでしょうか。アプリケーション・サーバーで使用されるデータベース・アカウントに削除権限を制限しないと、ゲームオーバーです。
-- 末尾はナイフのツイストです。ログ・ファイルで構文エラーが発生しても、残りのデリミタ文字が停止し、破壊が発生した場所が不明瞭化されるためです。
Java の PreparedStatement インタフェースは、この問題を回避します。このインタフェースは、文字列全体(正常か悪意かに関係なく)をwhere句で照合する文字として処理し、入力が不正である場合は安全に失敗します。
このようなSQLインジェクション攻撃は、おそらく小規模サイトで毎日行われているため、Open Web Application Security Projectの名高い OWASP Top 10 リスト に登場します。
最悪の習慣 #２: not-unit-testableテストを行わない
ユニットテストがない古い学校のプロジェクトを歩き回ったことがあります。
多くの古いアプリケーションは、1つのクラスで記述され、時にはワックスボール、オールインワンクラス、あるいはモンスタークラスと呼ばれていました。(ほんの少し丁寧な名前もあります。)単体テストは、定義上、1つの小さなコード単位をテストするように設計されているため、モンスター・クラスの単体テストを書くのは困難です。モンスター・クラスにはテストする小さなコードがないのです！テストがないだけでなく、コードもテスト可能として書き込まれません。
このようなアプリケーションを保守するように指示された場合は、テスト可能な小さな要素にモンスターを切り分け始めます。コード・クラスの大きさはどのくらいですか、それとも小さいですか。これは、マジック・サイズがなく、クラスやメソッドの正確なコード行数がないため、無限議論のトピックです。
単一責任の原則 (SRP) では、各クラスに1つの主な職責(計算の実行、オーダーの処理、結果の表示)があることが示されています。つまり、アプリケーションでこれら3つの処理を行う場合は、少なくとも3つのクラスが必要です。同様に、SRPは、メソッドが1つのことを行うべきであり、1つのことのみを行うべきであると述べています。
コードをモノリスから抽出し、ユニット・テストを記述し、それが合格していることを確認します。
もちろん、プロジェクトを最初から開始する場合は、コードの記述時にテストを記述する利点があります。テストを最初(つまり、テスト駆動型開発(TDD) —の方法に従って)記述すると、IDEはテスト対象のクラスとメソッドの概要を生成でき、最初から同期していることが保証されます。
最悪の習慣 #1: 空っぽの文書化されていないcatchブロック
次のコードは何を行いますか。
Connection jdbcConnection = getConnection();
var sqlQuery = "select id,name from student where name like 'Robert%'";
ResultSet rs = null;
try {
try {rs = jdbcConnection.createStatement().executeQuery(sqlQuery);
} catch (SQLException e) {}
while (rs.next()) {
int id = rs.getInt(1);
String name = rs.getString(2);
logger.log(id + " is " + name);
}
} catch (SQLException e) {
logger.log("SQL Error", e);
}
結果は、最初のSQL操作が成功したかどうかに依存します。操作が失敗した場合、例外は飲み込まれ、だれも賢くなることはありません。数行後、エラーを無視することは適切な方法ではないため、コードが再度問題になります。
この例は、数年前に作業したプロジェクトでチームが使用していたライブラリの実際のコード(記憶しておく必要のない名前)から抽出しました。ただし、実際のライブラリでは、不正に例外の取得しそれに対応するコードは数百行の間隔で、混乱全体がライブラリ呼び出しスタックで約20レベル停止していました。この混乱を見つけるのに何時間もかかりました。
つまり、例外を捕捉して無視するべきではないということです。例外を捕捉するか、捕捉しないかのいずれかです。例外を捕捉した場合は、その例外をいくつか実行します。少なくとも、例外をログに記録します。例外が重大な場合は、再スローするか、問題のコード・セクション全体から取り出します。
JDBCを処理する最新のフレームワーク(SpringやJakartaなど)の多くは、チェックされたSQL例外を捕捉し、未チェックの例外として再スローします。これにより、try-catch文またはthrows句の行や行を必要とするのではなく、それらをできるだけヒューマン・ユーザー近くで処理できます。
例外を無視しないというこのルールの1つの例外は、チェックされたInterruptedExceptionを持つThread.sleepです。シングルスレッド・コードでは、コメントを付けると例外を無視できる場合があります。
try {
Thread.sleep(5 * MSEC_PER_SEC);
} catch (InterruptedException ex) {
// Can't Happen - single threaded
}
番外編 最悪の習慣 ：警告を無視すること
IDEからの警告が表示されたら対処しましょう。コンパイラ警告は、いろいろなものが入った鞄のようです。重大なバグを示している場合もありますが、多くの場合無関係であり、重要な警告かどうか判断を下すために経験が必要です。
@SuppressWarnings annotation を正しく利用することで、無関係な警告を消すことができる場合があります。
対照的に、関連する警告はすぐに修正する必要があります。これは、警告を蓄積させると、チームが無視する習慣を身に付けるためです。そして、ある日、予想もしない時に、本当のバグが本番環境に紛れ込んでしまい、事後報告で、IDEが数週間前から警告していたことに誰かが気づくことになります。
さらに悪いことに、プロジェクトが警告のしきい値を超えると、遅すぎます。おそらく修正することはできないでしょう。
コードの品質は重要です。コードをきれいにしておいてください。あなたが救う開発者の仕事は、あなた自身かもしれません。