Javaのソースを添削！

java

id:s_kandaさんがJavaも始められたとのことで、Javaの添削or採点をお願いされました。仕事はJava一辺倒なんでここでボロを出したらやばいなぁ。とヒヤヒヤしてみたり。

今回はリファクタリングしてみて気づいたとこを書いてみる感じにしようと思います。で、早速リファクタリングした後のソースを。
main()が入ったHoneクラス

public class Hone {
    public static void main(String[] args) {
        HoneTime htime;
        if (args.length != 2) {
            System.out.println("使い方: java Hone sTime eTime");
            System.exit(0);
        }
        htime = new HoneTime(args[0], args[1]);
        System.out.println(htime.toString());
    }
}

HoneTimeクラス

import org.apache.commons.lang.StringUtils;

public class HoneTime {
	String s1, s2;

	// コンストラクタ
	public HoneTime(String s1, String s2) {
		this.s1 = s1;
		this.s2 = s2;
	}

	// 文字列表現
	public String toString() {
		String s = s1 + " - " + s2 + " === ";
		try {
			// スコープを移動。
			int m, m1, m2;
			if ((m1 = ji_to_fun(s1)) >= 0 && (m2 = ji_to_fun(s2)) >= 0) {
				m = m2 - m1;
				s += m + " min (" + (m / 60.0) + ")";
			} else {
				s += "時刻の入力が正しくありません。";
			}
		} catch (NumberFormatException e) {
			// 例外をキャッチし、メッセージを表示する。
			s += "時刻の入力が正しくありません。";
		}

		return s;
	}

	// 時刻を分に変換して返すメソッド
	private int ji_to_fun(String s) {
		int fun = 0;

		try {
			// StringUtilsを使用し、":"で文字列を分割
			String[] splitted = StringUtils.split(s, ":");

			// 文字列を数値に変換しつつ分を計算
			fun = Integer.parseInt(splitted[0]) * 60
					+ Integer.parseInt(splitted[1]);

		} catch (NumberFormatException e) {
			throw e;
		}

		return fun;
	}
}

あくまで個人が勉強時間を計るためのプログラムということで、機能はほとんど変えずに修正を加えてみました。
今回特に大きな変更は

  • 例外処理を追加
  • StringUtilsを導入

ですね。
まず、例外処理について。文字列をintに変換する作業をji_to_fun()内でInteger.parse()を使ってやっていて、せっかくNumberFormatExceptionをキャッチしているので例外らしく処理した方がいいかなと思いました。というわけでとりあえずcatchしたExceptionをthrowして、これを利用しているtoString()でcatchしてエラーメッセージを設定してみました。NumberFormatExceptionはRuntimeExceptionだからcatchはしなくても同じなのですが、明示的に書いておいてtoString()で処理することにしてみました。
同じくji_to_fun()内ですが、でStringUtilsを使って":"の前後の文字列を取得することにしました。Javaは文字列操作がそのままだと弱いのでJakartaで配布されているCommonsのStringUtilsなどを使います。
とりあえずこんなもので、あと気になるところは、命名規則的にメソッド名はji_to_fun()よりjiToFun()の方が「っぽい」気がします。また、toString()で、変数m,m1,m2はtryブロック内に入れちゃいました。こうするとどのスコープで使う変数なのか分かりやすいし、無駄も少ないかと。
「コンストラクタ内で分を求めたほうがいいのか？」という疑問があるようですが、これは別にしておいていいのではと思ってます。コンストラクタの機能をあまりリッチにすると先々変更が大変になったりする(拡張時に違う動作をさせるときに別のコンストラクタを用意しなければいけなくなったり)のでプロパティをセットする程度で充分でしょう。

あとはStringの連結に"+"を使わずにStringBufferのsubstring()を使うべきだという人もいたり、いろいろだと思いますが、細かくやっていくと思想とかになっていってしまうのでこの辺で。添削というか「自分ならこうする」に近いですね。採点は恐れ多いですけど、「合格点」じゃないでしょうか。

いやー、にしてもアーキテクトなんて名乗って(うさんくさいですが)仕事してるせいかクセで「多人数開発で混乱を少ないように」と思ってしまいますね。例外処理のあたりとか変更云々って思っちゃうのはその考え方のせいです。「なんか仕事っぽくてつまらない」って思っちゃったらすいません！でも、Javaは業務的な視点の強い言語だと思っているので一般的な考え方とも言えると思います。