CSVファイルから読み込んで郵便番号から住所を検索したい

###前提・実現したいこと
現在Javaでデータフォーマットの勉強をしています。
CSVファイルから郵便番号を読み込んで、一致する住所を表示するプログラムを書いたのですが、もっとこうすれば分かりやすいなど改善点があれば教えていただきたいです。

###該当のソースコード

Java
1
2import java.io.FileNotFoundException;
3import java.io.IOException;
4import java.nio.charset.Charset;
5import java.nio.file.Files;
6import java.nio.file.Paths;
7import java.util.HashMap;
8import java.util.List;
9import java.util.Scanner;
10
11public class CsvPractice {
12	
13	static HashMap<Integer, String> zipCodeDataMap = new HashMap<>();
14
15	public static void main(String[] args) {
16		CsvPractice cp = new CsvPractice();
17		cp.readCSV();
18
19		while (true) {
20			System.out.print("郵便番号：");
21			Scanner scanner = new Scanner(System.in);
22
23			try {
24				String postalCodeStr = scanner.next();
25
26				if (postalCodeStr.equals("exit")) {
27					System.out.println("終了します");
28					return;
29				}
30
31				Integer postalCode = Integer.valueOf(postalCodeStr);
32				String Address = zipCodeDataMap.get(postalCode);
33				if (Address != null) {
34					System.out.println("郵便番号 : " + postalCode);
35					System.out.println("住所    : " + Address);
36				} else {
37					System.out.println("郵便番号に対応する住所はありません");
38				}
39
40			} catch (NumberFormatException e) {
41				System.out.println("郵便番号を入力してください");
42				continue;
43			}
44
45		}
46	}
47
48	/**
49	 * csvファイルからの読み込み
50	 * 
51	 */
52
53	void readCSV() {
54
55		try {
56			List<String> readLineList = Files.readAllLines(Paths.get("data/40FUKUOK.CSV"), Charset.forName("SJIS"));
57			for (String readLine : readLineList) {
58				String[] values = readLine.split(",");
59				if (values == null || values.length != 15) {
60					System.out.println("エラー");
61					continue;
62				}
63				Integer PostalCode = Integer.valueOf(values[2].replaceAll("\"", ""));
64				String Address = values[6].replaceAll("\"", "") + values[7].replaceAll("\"", "")
65						+ values[8].replaceAll("\"", "");
66				zipCodeDataMap.put(PostalCode, Address);
67			}
68
69		} catch (FileNotFoundException e) {
70			e.printStackTrace();
71		} catch (IOException e) {
72			e.printStackTrace();
73		}
74
75	}
76}

###補足情報(言語/FW/ツール等のバージョンなど)
Java eclipse

行動規範の内容に同意します

回答5件

ベストアンサー

「分かりやすい」
という観点からは、特に問題ないと思います。

以下、その他の観点について気づいた点をコメントさせていただきます。

scanner 変数について

Scanner.next()メソッドは繰り返し実行できますので、while文の内部でnew Scanner(System.in)を実行する必要はありません。
while文の直前に 1度だけ実行すれば十分です。

また、現状のプログラムはScanner.close()を実行していないため、リソースリークを引き起こす可能性があります。
https://en.wikipedia.org/wiki/Resource_leak

JDK1.7以上を使用しているのであれば、以下のようにtry-with-resources文を使用すると良いでしょう。

java
1try (Scanner scanner = new Scanner(System.in)) {
2    while (true) {
3        System.out.print("郵便番号：");
4
5        try {
6            String postalCodeStr = scanner.next();
7
8            ...
9
10        } catch (NumberFormatException e) {
11            System.out.println("郵便番号を入力してください");
12            continue;
13        }
14
15    }
16}

String.split() メソッドについて

CSVファイルから読み込んだ行のなかに

,,1300015,,,,東京都,墨田区,横網,,,,,,

のように、15番目の要素（最後の要素）が空の行があると、
このプログラムは「エラー」と表示してその行を読み飛ばしてしまいます。

なぜなら、引数を 1つだけ取るsplit()メソッドは、
左から見て最後の空でない要素（上の例の場合、「横網」）以降の空の要素を、結果の配列に含まないからです。
https://docs.oracle.com/javase/jp/7/api/java/lang/String.html#split(java.lang.String)

結果として得られる配列には後続の空の文字列は含まれません。

最後の要素に空文字を許容しない場合は今のままで問題ありませんが、
そうでない場合、以下のように第二引数に負の値を指定してやると良いでしょう。

java
1String[] values = readLine.split(",", -1);

https://docs.oracle.com/javase/jp/7/api/java/lang/String.html#split(java.lang.String, int)

limit パラメータは、このパターンの適用回数を制御するため、結果として得られる配列の長さに影響を及ぼします。

（中略）
n が負の値の場合、このパターンの適用回数と配列の長さは制限されません。

また、split() メソッドの戻り値が null になることはありません。
上と同じリンク

入力されたどの部分とも式が一致しない場合、配列は 1 つの要素 (つまり、この文字列) だけを保持します。

そのため、入力チェックのvalues == nullは不要です。

String.replaceAll() メソッドについて

このメソッドは、指定した正規表現に一致する文字列を置換するメソッドです。
https://docs.oracle.com/javase/jp/7/api/java/lang/String.html#replaceAll(java.lang.String, java.lang.String)

指定された正規表現に一致する、この文字列の各部分文字列に対し、指定された置換を実行します。

このプログラムでは単に"を削除するだけで正規表現が必要なわけではないので、String.replace() メソッドで十分です。
https://docs.oracle.com/javase/jp/7/api/java/lang/String.html#replace(java.lang.CharSequence, java.lang.CharSequence)

リテラルターゲットシーケンスに一致するこの文字列の部分文字列を、指定されたリテラル置換シーケンスに置き換えます。

readCSV() メソッド内の catch句について

FileNotFoundExceptionなどの例外をcatchしていますが、結局e.printStackTrace()を実行するだけのため、
このメソッドの呼び出し側（このプログラムの場合、main()メソッド）からは、メソッドが正常に実行できたかどうかを知る術がありません。

実際、readCSV() メソッド内で例外が発生しても、このプログラムは何事もなかったかのように郵便番号の入力を要求してしまいます。

以下のように、発生した例外をthrowして、メソッドの呼び出し側にエラーが起きたことを検知できるようにしてやったほうが、
メソッドの設計として優れていると私は考えます。

java
1void readCSV() throws Exception {
2
3    try {
4
5        ...
6
7    } catch (IOException | NumberFormatException e) {
8        e.printStackTrace();
9        throw e;
10    }
11
12}

zipCodeDataMap フィールドについて

同じクラスの中に定義してあるとはいえ、インスタンスメソッド(readCSV()）がスタティックフィールド(zipCodeDataMap)の状態を変更するのは、
クラス設計の観点からはよろしくありません。
http://findbugs.sourceforge.net/bugDescriptions.html#ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD

This is tricky to get correct if multiple instances are being manipulated, and generally bad practice.

このプログラム自体は問題なく動作しますが、今後、別のプログラムを作成する際には注意してみることをお勧めします。

投稿2017/06/12 08:12

KiyoshiMotoki

総合スコア4791

momon-ga

2017/06/12 10:50

そうそう。staticで定義してるが気持ち悪いというのを指摘し忘れてました。インスタンスごとにcsvのデータを持てないので部品として使えないなぁと思ってました。とはいえ、Practiceなので・・・まぁいっかとも。

退会済みユーザー

2017/06/14 13:28

細かく説明ありがとうございます。非常に参考になりました。

行動規範の内容に同意します

そんなに問題ないと思います。

私が変更をするとしたら。

リストファイル名の固定をやめる。

コンストラクタか、readCSVの引数で設定する。

CSVの構造のマジックナンバーを削除

郵便番号のデータファイルの形式等
にある定義をして、15や2、6、7、8を利用しないようにします。

Addressの保持方法

連結したものをもたずに生のString配列で保持をして、取得時に加工するようにします。
ちなみに、変数名(address)のように小文字から始めることが慣例的に多いと思います。

zipCodeDataMapへのアクセッサー

生データを返す場合(get)や、元の実装のように加工して出す場合(getAddress)用に、生のHashMap
をさわらせるのでなく、アクセッサーを用意します。
まぁクラス名がPracticeなところから汎用性は考えてないとは思いますが・・・

ダブルクォートの削除について

まぁ、みればわかるのですが、メソッドを切り出すとロジックに名前がつけれます。

 String address = values[6].replaceAll("\"", "") + ...

 よりも

 String address = cutQuote(values[CsvPractice.PREFECTURES])
                 + cutQuote(values[CsvPractice.xxxxx]) ...

みたいな感じです。

投稿2017/06/12 06:25

momon-ga

総合スコア4820

退会済みユーザー

2017/06/14 13:30

回答ありがとうございます。参考にさせていただきました。

行動規範の内容に同意します

CSVファイルの名称を見るに、日本郵便（郵便局）の提供している郵便番号データを使用していると推察します。

あれ、単純に１行で収まりきらないことがあるので、それも加味しないと正確性が落ちます。
対象が福岡県だと、826-0043 とか複数行になってなかったかな。

投稿2017/06/12 06:21

tacsheaven

総合スコア13703

KSwordOfHaste

2017/06/12 06:56

手元のをみたら重複してました！ 40206,"826 ","8260043","ﾌｸｵｶｹﾝ","ﾀｶﾞﾜｼ","ﾅﾗ(ｱｵﾊﾞﾁｮｳ､ｵｵｳﾗ､ｶｲｼｬﾏﾁ､ｶｽﾐｶﾞｵｶ､ｺﾞﾄｳｼﾞﾆｼﾀﾞﾝﾁ､ｺﾞﾄｳｼﾞﾋｶﾞｼﾀﾞﾝﾁ､ﾉｿﾞﾐｶﾞｵｶ､","福岡県","田川市","奈良（青葉町、大浦、会社町、霞ケ丘、後藤寺西団地、後藤寺東団地、希望ケ丘、",0,0,0,0,0,0 40206,"826 ","8260043","ﾌｸｵｶｹﾝ","ﾀｶﾞﾜｼ","ﾏﾂﾉｷ､ﾐﾂｲｺﾞﾄｳｼﾞ､ﾐﾄﾞﾘﾏﾁ､ﾂｷﾐｶﾞｵｶ)","福岡県","田川市","松の木、三井後藤寺、緑町、月見ケ丘）",0,0,0,0,0,0

tacsheaven

2017/06/12 07:03

この場合、町域部分だけ次の行と合体させて処理しないといけないんですよ。奈良（青葉町、大浦、会社町、霞ヶ丘、後藤寺西団地、後藤寺東団地、希望ヶ丘、松の木、三井後藤寺、緑町、月見ヶ丘）となるのですが、本来これは（）内を、で分割して、奈良青葉町、奈良大浦…のように分けるべきだったりします（つまり１郵便番号に対して複数町域が該当する）

KSwordOfHaste

2017/06/12 07:08 編集

自分の回答にも書きましたが、やはり実際に使う場合は入力補助の意味合いが強いですよね・・・（一意に決まるわけではないという点で）

退会済みユーザー

2017/06/14 13:32

回答ありがとうございます。仰る通り日本郵便が提供しているファイルです。郵便番号が重複する場合の処理なども今後考え実装していきたいと思います。

行動規範の内容に同意します

同じく斜め下に(ω・

readCSV() のエラー判定で、エラーが出た時に、CSVのどの行でエラーになっているかがわからなくなりますので、CSVのエラー行を出すと良いでしょうか。

あとは readCSVのreturnする結果は void ではなく、HashMap<Integer, String> で返すようにし、実行側でこれを受け取るほうが何かと拡張しやすいと思います。

投稿2017/06/12 05:57

A-pZ

総合スコア12011

退会済みユーザー

2017/06/14 13:32

回答ありがとうございます。今後実務で使う上で拡張性なども意識していきたいと思います。

行動規範の内容に同意します

斜め下の回答ですが・・・

CharsetにSJISを使っておられます。日本の住所にはJIS第一・第二水準に含まれない漢字を用いたものもあると聞きます。よってCharsetは使えない漢字があるかどうか気にしなくても済むUTF-8、もしくはせめてWindows_31Jにしておいてはいかがでしょうか。

稀少地名漢字リスト

訂正：tacsheavenさんにご指摘いただいたように、郵便局提供のCSVならSJISでよさそうです。失礼いたしました。

このままでは回答が空になるので、さらに斜め下のトリビア的な話を・・・

郵便局の住所ファイルには若干落とし穴があると聞いたことがあります。住所が「ＸＸ周辺」というような変則的な表記が一部にあるそうです。どのみち町名までしか出ない入力補助的な意味合いなので、人が編集する際に訂正すれば済むだけの話ではあります。また郵便番号に確か一部重複があったと思います。あまり役に立つ情報でなくて恐縮ですが・・・

投稿2017/06/12 05:35

編集2017/06/12 06:49

KSwordOfHaste

総合スコア18394