レビューで鍛えるJavaコーディング力その１（日付バリデーション）

現場でJavaのソースをレビューしていると、一目で「問題だ」と気づくコードによく出会います。
しかも、同じような問題をアチコチで見かけることも、少なくありません。

FindBugsを導入したおかげで、そういう問題が多少減ったものの、
やはりゼロになるわけではありません。

、、、ということで、
そんな問題コードに即座に反応できるようになるために、
私が見てきた問題コードをクイズ形式で紹介していきます。

ぜひ皆さんも、脊髄反射でNG部分を見つけてください。
見つけられなければ、適当にブクマでもしてください（笑

では今回は、日付チェックに関する問題です。

問題

以下のコードの問題を指摘し、修正してください。
ただし、問題は複数あることもあれば、全くないこともあります。

/**
 * 日付が正しい形式であり、存在する日付であることを検証します。
 *
 * @param date 日付を示す文字列
 * @return 正しい日付の場合はtrue、そうでない場合はfalseを返します。
 */
private static final DateFormat DF = new SimpleDateFormat("yyyy/MM/dd hh:mm:ss");

public static boolean checkDate(String date)
{
	try
	{
		DF.parse(date);
	}
	catch (ParseException ex)
	{
		return false;
	}

	return true;
}

シンプルな日付のバリデーションメソッドです。
呼び出し元では、この値を使ってCSV出力などを行うという想定です。

解答１

真っ先に思いつくのは
「SimpleDateFormatはスレッドセーフじゃないから、クラス変数として持っちゃダメ」
「setLenientしていないから、暦のチェックがされない」
の、どちらかではなかったでしょうか。

SimpleDateFormatが日付のパースやフォーマットを行うクラスということしか知らず、
どこかで見たサンプルを丸写ししているレベルだと、この問題に引っかかります。

SimpleDateFormatは、複数スレッドから同時にアクセスされてしまうと、
formatした結果がおかしくなったり、例外が発生することがあります。

なので、毎回SimpleDateFormatのインスタンスを生成するか、
Jakarta Commons LangのFastDateFormatクラスを利用しましょう。

また、DateFormat#setLenient(false)を指定していない場合
「2009/12/32 10:89:50」のような存在しない日付もパースし、
「2010/01/01 11:29:50」として扱ってしまいます。

目的がバリデーションである以上は、
「2009/12/32 10:89:50」はエラーとすべきでしょう。

だから、DateFormatの宣言部分は、以下のように修正すべきです。

public static boolean checkDate(String date)
{
	DateFormat df = new SimpleDateFormat("yyyy/MM/dd hh:mm:ss");
	df.setLenient(false);

	try
	{
		df.parse(date);
	}
	// 後は同じ
}

楽勝でしたか？

解答２

もう一つ、大きな間違いがあります。
それは "yyyy/MM/dd hh:mm:ss" です。

hhは時間を示す表示ですが、AM/PMがつくことを前提とした「12時間表記」なのです。
これでは「23:00:00」をエラーとみなしてしまうか、「11:00:00」と扱ってしまいます。

24時間表記は「hh」ではなく「HH」なので
"yyyy/MM/dd hh:mm:ss" ではなく "yyyy/MM/dd HH:mm:ss" と記述する必要があるのです。

かなりうっかり度が高いミスですが、
Javaを始めて1〜2年経って、日付フォーマットやAPIを大体覚えてしまった人や、
特に問題コードを見てスレッドセーフやsetLenientの問題に脊髄反射した人ほど、
このミスを見落としやすかったのではないでしょうか。

解答３

まだ問題があります。
それは、SimpleDateFormat付フォーマットが前方一致であるため、
後ろに余計な文字列がついていても、パースできてしまう、という問題です。

たとえば "2010/01/01 11:29:50ですよ" とか
あるいは "2010/01/01 11:29:5a" なども、パースできてしまいます。

特に後者はJakartaのCommons Validatorを使っていても
チェックができなかったりするので、注意が必要です。

「入力は寛容に、出力は厳格に」の原則に従えば
別にパースできてしまっても良いじゃないか、という想いもありますが、
これはCSV出力するためのフォーマットチェックと位置づけていますから
厳密にチェックする必要があるでしょう。

要するにDateFormatで行うのは「暦の妥当性」チェックであって、
形式チェックはきちんと別に行う必要がある、ということです。

正規表現で形式チェックを行ったり、
長さチェックとsubstringとparseIntでのチェックを行なっても良いでしょう。

私は時々妥当性チェックとして、
「パースとフォーマットを行って、元の文字列を取得できるか」を確認する、
という方法を使うことがあります。

こんな感じです。

public static boolean checkDate(String date) {
	DateFormat df = new SimpleDateFormat("yyyy/MM/dd hh:mm:ss");
	String converted = null;
	try
	{
		Date convertedDate = df.parse(date);
		converted = df.format(convertedDate);
	} catch (ParseException ex) {
		return false;
	}

	return converted.equals(date);
}

パフォーマンスには疑問があるかも知れませんが、
setLenientの存在や、SimpleDateFormatが前方一致であることを知らなくても
この方法であれば、確実に日付をチェックすることができます。

この「変換と逆変換を行なって元の値を取得できるか」という可逆性チェックは
APIの細かい仕様を知らなくても、チェックできる、
つまり「安全性」というメリットがあるのです。

まとめ

それでは、今回の問題をおさらいしておきましょう。

SimpleDateFormatはスレッドセーフではない。毎回インスタンス生成するか、FastDateFormatを使う。
暦のチェックをするなら、setLenient(false)は必須。
hh:mm:ssではなくHH:mm:ssが正しい。
SimpleDateFormatは前方一致。
parseとformatをして、元の値が取得できるかをチェックするのは良いアイデア。

こんな感じで、これからもちょくちょく問題コードを並べてみますね！

谷本心 in せろ部屋

はてなダイアリーから引っ越してきました

レビューで鍛えるJavaコーディング力その１（日付バリデーション）

問題

解答１

解答２

解答３

まとめ