トップ > Java

Hudson の Git Plugin を使うと文字化けする問題とその解決方法 (不完全)

Hudson Git Java

こちらも仕事で Hudson と Git を使い始めた頃から気付いたんですが、ちょうどいい機会なので直してみます。
文字化けするのは、Hudson の Web 画面から確認できるコミットメッセージです。

始める前に

ここで紹介する方法は、プラグインのクラスファイルの一部を入れ替える方法です。
あくまでその場しのぎの解決方法であることを理解したうえで、この方法を実行する場合は自己責任でお願いします。

調査

まず、本来あるべきコードと、認識されているコードを調査しました。

Googleの文字化け・まとめ（文字化けパターン）

上のページによると、UTF-8 なのに Shift_JIS として認識しているようです。
Git でのデフォルトのコミットメッセージは UTF-8 なので、これを読み込む際に間違ったエンコード方式を指定している可能性が濃厚です。


そこで、Git plugin のコード*1 *2を落としてコミットメッセージを読み込んでいる部分を探しました。

原因

文字化けの原因は、GitChangeLogParser クラスでファイルの読み込みに FileReader クラスを使っていることでした。
FileReader クラスは手軽にファイル読み込みができますが、読み込む際のエンコーディングは指定できず、常にシステムのデフォルトエンコーディングが使用されます。
このため、Windows で Hudson を動かし、さらに Git まで使っているという変態構成だと、ファイルのエンコーディングが UTF-8 であっても MS932 を使ってしまい、文字化けが発生していました。


これを回避するためには、i18n.commitencoding を Shift_JIS にすることも考えられるのですが、すでに utf-8 で運用しているため、できればこれは避けたいです。

GitChangeLogParser.java の修正

本来なら、リポジトリのコミットメッセージのエンコーディング方式を設定出来るように修正するのが一番なのですが、Hudson で Plugin を作ったことがないため、手っ取り早い方法を取りました。
自分が使っているリポジトリの i18n.commitencoding はすべてデフォルト状態、つまり utf-8 なので、FileReader を使わず、InputStreamReader と FileInputStream を組み合わせて utf-8 で読み込むようにしました。

BufferedReader rdr = null;
try {
    rdr = new BufferedReader(new InputStreamReader(new FileInputStream(changelogFile), "utf-8"));
    ...
} finally {
    if (rdr != null) rdr.close();
}

try の外側で BufferedReader と FileReader を new していた部分も、try の中で行うように変更しています。
また、import も変更しています。


後はこれをコンパイルして Hudson をインストールした場所にある plugins/git/WEB-INF/classes/hudson/plugins/git に生成された .class ファイルをコピーすればいい・・・のですが、NUnit Plugin の場合と違ってこちらは hudson.model.AbstractBuild と hudson.scm.ChangeLogParser というクラスを使用しているので、その辺もよろしくやってあげる必要があります。
使用している hudson.war から hudson-core-バージョン.jar を取り出し、src\main\java に置きます。

javac -cp hudson-core-1.355.jar hudson\plugins\git\GitChangeSet.java hudson\plugins\git\GitChangeSetList.java hudson\plugins\git\GitChangeLogParser.java

とするとコンパイルできました*3。


これで、コミットメッセージが文字化けせずに表示されるようにな・・・ると思ったのですが、微妙に化けてしまいます。
jobs フォルダの中の「プロジェクト名\builds\ビルド日時\changelog.xml」を utf-8 で開いてみると、どこかで変換にミスっているのか、? という文字が目立ちます。
この changelog.xml をどこで出力しているのかわからないので、これ以上は分かりませんでした*4。

*1:https://hudson.dev.java.net/svn/hudson/trunk/hudson/plugins/git/

*2:Github にもコードがあるらしい。http://github.com/magnayn/Hudson-GIT-plugin

*3:これはバージョンが 1.355 の場合です

*4:Git plugin の中を FileWriter とか BufferedWriter で検索したけど見つからなかった

Hudson の NUnit Plugin を使うとテストケースの数が減る問題とその解決方法

Hudson Java C#

仕事で Hudson を使い始めた頃 (去年の 10 月だか 11 月だか) から、NUnit のテストケースが実際よりも少なくなってしまう問題は認識していましたが、あまり気にしていませんでした。
しかし今回、かなりの数のテストケースが削られていたため、ちょっと調べて直してみました。

始める前に

ここで紹介する方法は、プラグインのクラスファイルの一部を入れ替える方法です。
あくまでその場しのぎの解決方法であることを理解したうえで、この方法を実行する場合は自己責任でお願いします。

調査

まず、どこでテストケースが減っているのかを調べていきました。


NUnit が出力する XML ファイルでは、当然ながらテストケースの数は正しいものが出力されていました。
どうやら NUnit Plugin が NUnit の出力する XML ファイルを JUnit の形式に XSL ファイルで変換しているらしいので、この XSL ファイルに NUnit から出力された XML ファイルを与え、変換してみました。しかし、ここではテストケースの数は正しいままでした。それに、XSL ファイルの内容を見ても、テストケースの数は減りそうにありません。


次に、ソース *1 をチェックアウトして、変換部分を探してみると、NUnitReportTransformer クラスの splitJUnitFile メソッドが怪しい感じでした。
このメソッドは XSL 変換した結果を複数ファイルに分割しているのですが、なぜ分割しているのかは分かりません。
しかし、ここで分割にミスってテストケースを減らしている可能性は大いにあり得ますので、実際にこのクラスを動かしてみると、やはりテストケースの数が減っていました。

原因

どのテストケースが減っているのかを調べたところ、NUnit のパラメタライズドテストを使い、テストメソッドをオーバーロードしていた部分が減っていました。

[TestCase(10, "hoge")]
[TestCase(20, "piyo")]
public void Hoge(int input, string expected) { ... }

[TestCase(10, true, "hoge")]
public void Hoge(int input, bool isXxx, string expected) { ... }

NUnit のパラメタライズドテストの詳細については、

を参照してください。


上記のテストコードが、HogePiyo 名前空間の FooBarTest クラスに記述されていた場合、全体としては 3 つのテストケースが出力されて欲しいにも関わらず、NUnit Plugin ではテストメソッド名しか考慮しておらず、1 つのテストケースしか出力されていませんでした (後のテストケースで上書きされる)。
さらに、NUnit が出力する XML で、パラメタライズドテストは test-suite 扱いだったので、splitJUnitFile の動作と合わさって、テストケースが削られていました。


例えば、上の例で出力される XML は

<test-suite name="HogePiyo" ...>
 <test-suite name="FooBarTest" ...>
  <test-suite name="Hoge" ...>
   <results>
    <test-case name="HogePiyo.FooBar.Hoge(10, &quot;hoge&quot;)" .../>
    <test-case name="HogePiyo.FooBar.Hoge(20, &quot;piyo&quot;)" .../>
   </results>
  </test-suite>
  <test-suite name="Hoge" ...>
   <results>
    <test-case name="HogePiyo.FooBar.Hoge(10, True, &quot;hoge&quot;)" .../>
   </results>
  </test-suite>
 </test-suite>
</test-suite>

のようなものになりますが、

[Test]
public void HogeTest1() { ... }

[Test]
public void HogeTest2() { ... }

[Test]
public void HogeTest3() { ... }

の場合、

<test-suite name="HogePiyo" ...>
 <test-suite name="FooBarTest" ...>
  <results>
   <test-case name="HogeTest1" .../>
   <test-case name="HogeTest2" .../>
   <test-case name="HogeTest3" .../>
  </results>
 </test-suite>
</test-suite>

のような XML になります。

XSL の修正

上の XML をそれぞれ XSL 変換すると、

<!-- パラメタライズドテストの場合 -->
<testsuite name="HogePiyo.FooBarTest.Hoge" ...>
 <testcase classname="HogePiyo.FooBarTest.Hoge" name="" .../>
 <testcase classname="HogePiyo.FooBarTest.Hoge" name="" .../>
</testsuite>
<testsuite name="HogePiyo.FooBarTest.Hoge" ...>
 <testcase classname="HogePiyo.FooBarTest.Hoge" name="" .../>
</testsuite>

<!-- 普通のテストの場合 -->
<testsuite name="HogePiyo.FooBarTest" ...>
 <testcase classname="HogePiyo.FooBarTest" name="HogeTest1" .../>
 <testcase classname="HogePiyo.FooBarTest" name="HogeTest2" .../>
 <testcase classname="HogePiyo.FooBarTest" name="HogeTest3" .../>
</testsuite>

となります。
パラメタライズドテストでは classname にメソッド名まで含んでしまっている上、name が空になってしまっています。


この問題を修正するために、main/resources/hudson/plugins/nunit/nunit-to-junit.xsl の 13 行目の XSL 変数 assembly と 18 行目の testcaseName で直接値を決定していた部分を、仮の testcaseName の内容を見てから決定するようにしました。

<!-- この下が13行目 -->
<xsl:variable name="tmpAssembly"
 select="concat(substring-before($firstTestName, @name), @name)" />     <!-- assemblyからtmpAssemblyに変更 -->

<!--  <redirect:write file="{$outputpath}/TEST-{$tmpAssembly}.xml">-->

 <testsuite name="{$tmpAssembly}"
  tests="{count(*/test-case)}" time="{@time}"
  failures="{count(*/test-case/failure)}" errors="0"
  skipped="{count(*/test-case[@executed='False'])}">
  <xsl:for-each select="*/test-case[@time!='']">
   <xsl:variable name="tmpTestcaseName">    <!-- testcaseNameからtmpTestcaseNameに変更 -->
    <xsl:choose>
     <xsl:when test="contains(./@name, $tmpAssembly)">
      <xsl:value-of select="substring-after(./@name, concat($tmpAssembly,'.'))"/>
     </xsl:when>
     <xsl:otherwise>
      <xsl:value-of select="./@name"/>
     </xsl:otherwise>
    </xsl:choose>
   </xsl:variable>
   <!-- 以下新規追加するコード -->
   <xsl:variable name="assembly">           <!-- ここでassemblyを決定 -->
    <xsl:choose>
     <xsl:when test="string-length($tmpTestcaseName)=0">
      <xsl:value-of select="substring-before($tmpAssembly, concat('.', ../../@name))"/>
     </xsl:when>
     <xsl:otherwise>
      <xsl:value-of select="$tmpAssembly"/>
     </xsl:otherwise>
    </xsl:choose>
   </xsl:variable>
   <xsl:variable name="testcaseName">       <!-- ここでtestcaseNameを決定 -->
    <xsl:choose>
     <xsl:when test="string-length($tmpTestcaseName)=0">
      <xsl:value-of select="concat(../../@name, substring-after(./@name, ../../@name))"/>
     </xsl:when>
     <xsl:otherwise>
      <xsl:value-of select="$tmpTestcaseName"/>
     </xsl:otherwise>
    </xsl:choose>
   </xsl:variable>

仮の testcaseName (tmpTestcaseName) が空の場合、パラメタライズドテストとみなして、assembly と testcaseName を組み直し、空ではない場合、tmpAssembly と tmpTestcaseName を assembly と testcaseName として使用するようにしました。


しかし、これだけでは XSL 変数 assembly の名前がかぶった場合、つまりパラメタライズドテストでオーバーロードしていた場合に、テストケースが消えてしまいます。
なぜなら、main/java/hudson/plugins/nunit/NUnitReportTransformer.java の splitJUnitFile メソッドが

String filename = JUNIT_FILE_PREFIX + element.getAttribute("name").replaceAll(ILLEGAL_FILE_CHARS_REGEX, "_") + JUNIT_FILE_POSTFIX;
File junitOutputFile = new File(junitOutputPath, filename);
FileOutputStream fileOutputStream = new FileOutputStream(junitOutputFile);

となっているからです。
element は testsuite 要素が格納されているので、その name 属性、つまり XSL 変数 assembly の名前がかぶっていた場合、上記プログラムの 1 行目で生成されるファイル名は同一のものになります。
そして、上記プログラムの 3 行目で FileOutputStream のコンストラクタを呼び出していますが、ここで同じ名前のファイルが上書きされてしまい、その分のテストコードが消えることになります。

NUnitReportTransformer.java の修正

これを避けるために、NUnitReportTransformer.java にも修正が必要となります*2。
要は、作成しようとしているファイル名がすでに存在するなら、違うファイル名を使えばいいだけです。


そこで、ファイル名の末尾に連番を付与することにしました。
上記 3 行は、以下のように変更します。

File junitOutputFile = outputFile(element.getAttribute("name").replaceAll(ILLEGAL_FILE_CHARS_REGEX, "_"), junitOutputPath);
FileOutputStream fileOutputStream = new FileOutputStream(junitOutputFile);

そして、フィールドとメソッドを追加します。

private static int seq = 2;
private static File outputFile(String tmp, File parent) {
    File f = new File(parent, filename(tmp));
    return f.exists() ? outputFileImpl(tmp, parent)
                      : f;
}
private static File outputFileImpl(String tmp, File parent) {
    File f = new File(parent, filename(tmp, seq));
    if (f.exists()) {
        seq++;
        return outputFileImpl(tmp, parent);
    }
    seq = 2;
    return f;
}
private static String filename(String name) {
    return JUNIT_FILE_PREFIX + name + JUNIT_FILE_POSTFIX;
}
private static String filename(String name, int n) {
    return JUNIT_FILE_PREFIX + name + "_" + n + JUNIT_FILE_POSTFIX;
}

後はこれをコンパイルして、Hudson をインストールした場所にある plugins/nunit/WEB-INF/classes/hudson/plugins/nunit に .xsl ファイルと生成された .class ファイルをコピーします。
これで、ファイル名がかぶることがなくなり、テストケースの数が減ることがなくなりました。
さらに、今まで空だったテスト名や、メソッド名まで含んでしまっていたクラス名も直りました。

別の問題

動作には支障がないので、問題ないと言えば問題ないのですが、ひとつ気になるところを見つけてしまいました。
NUnitReportTransformer.java の 34 行目の正規表現なのですが、

[\\*/:<>\\?\\|\\\\\";]+

となっています。文字クラスの中では * も ? も | も特別な意味を持ちませんので、

[*/:<>?|\\\\\";]+

で大丈夫です。\ が続いているのがちょっと読みにくいので、

[\"*/:<>?|\\\\;]+

と、離してしまうとより読みやすくなると思います。

*1:https://hudson.dev.java.net/svn/hudson/trunk/hudson/plugins/nunit/

*2:XSL だけでどうにかできそうですが、考えるのが面倒だったので・・・

TDD Boot Camp の参加報告とか読んで

TDD Java C#

TDD Boot Camp には行っていないんだけど、参加者のエントリを色々読んで触発されたので思っていることをちょこっと書いておきます。
日曜日は id:a-hisame に無理言って色々と聞いた*1しね！
以下引用が多くて微妙に長文。

アクセス修飾子

  • デモ：coberturaに機能追加する*1
    1. テストできそうな箇所を小さい範囲にメソッド抽出
    2. さらに、副作用がある箇所をprotectedメソッドに抽出
    3. サブクラスで副作用メソッドをオーバーライドして無効化
    4. テストのために、検出用変数をprivateからpublicに変更
    5. 検出用変数にアクセスして、assertを記述


*1: この辺ちょっとうろ覚え。もし間違っていたらご指摘ください。

TDD Boot Campに参加しました - @ikikko のはてなブログ

これの 4 と 5 なんだけど、個人的には package private でいいんじゃないかな？と思う。
パッケージ単位ってそこそこ扱いやすい粒度だし、もしそうじゃないならそのパッケージに詰め込みすぎってサインだとも考えられるし。

最後に質問が出ましたが、テストのためにprivateをpublicに変更した場合どうするかという話で、リファクタリングを(おそらく複数サイクル)行い、再びprivateになるようにコードを改善する、という答えを頂きました。

TDD Boot Camp体験記 - Logic Dice

ふむ・・・
つまり private に戻す段階で、検出用変数にアクセスするテストを削除する、ということだと思うんだけど、ここですよね。
package private にしておくと、削除しなくてもコンパイルはでき、テストも Green のままに保てるけど、「public から private に戻す」というステップを踏まなくなる。
すると、テストは内部の実装に依存したままになり、後々まずくなる・・・かもしれない。
うーん、ここら辺はもうちょっと考えないといけないかな。


あとは削除対象のテストが上位の (検出用変数にアクセスする必要のない) テストでカバーできているということをどう判断するか、かな。

テストコードのリファクタリング

「テストは分かりやすく書く物であるが、同じような処理をメソッド化しても良いか」という疑問をぶつけて見ました。
ここでの同じような処理のメソッド化とは、上のLRUキャッシュの例であれば、複数のデータを纏めてput出来るような可変長引数をとるメソッドを作ってもよいのかという点です。
結論から言えば「メソッド化するべき」です。そもそも、テストに対してもリファクタリングを行う以上、このような結果になります。
ただし、そのメソッドは明瞭で単純で無ければなりません。また、コレクションなどの処理を行う場合にたまたまテストが成功していないか(例えば、ループを書いているが全くループせず空の要素を作ってしまい、その結果テストが成功する)ということに注意を払う必要があります。

TDD Boot Camp体験記 - Logic Dice
  • テストコードも、プロダクトコードと同様に無駄なコードは書かないこと。重複を消す。DRY。コピペ禁止。で、見通しがよくなる。

Mapを扱うお題だけどvalueの値自体はテストに関係ないので、put(key, key)みたいなヘルパーメソッドを作る

TDD Boot Campに参加しました - @ikikko のはてなブログ

テスト用の単純なヘルパメソッドはよく作るんだけど、これはいいのかな？と思っていたので一安心。
プリミティブ型の可変長引数受け取るメソッドから、実際に必要なオブジェクトを格納するコレクション生成したりとか、よくやります。

// C#
static IEnumerable<Hoge> Hs(params double[] hs)
{
    foreach (var h in hs) yield return new Hoge(h);
    // 最近は
    // return hs.Select(h => new Hoge(h)).ToArray();
    // みたいなコードを書くことも
}

みたいな。

しばらく Red なゴール

「シナリオテストをTDDの最初に書いてもいいのか」という点です。
〜略〜
結論としては「良い」という意見を頂きました。もちろん、そんな理想系が最初から通ることはあり得ないので、@Ignoreアノテーションをつけるなどして必要な時まで無効化しておく必要はあります。こういったクラス作成の「ゴール」を決めておくことは非常に有用です。
ただし、それがただ１つのゴールではありません。リファクタリングなどによって、ゴールが形を変えてしまうかも知れません。その時には、その変化を受け入れる必要があります。

TDD Boot Camp体験記 - Logic Dice

あらかじめ条件が明確化されているものについては、受け入れテスト的な「しばらくはREDなゴール」を書いてもよい。また、開発を進める中でそのゴールを修正してもよい。

NUnit では、そう言う「しばらく Red なゴール」に対して、Explicit 属性を付けておくと良いのかな、と思った。
Explicit 属性は「明示的にそのテストを実行しない場合に無視される」属性で、例えばプロジェクト全体をテストする場合は無視される。
ただし、Ignore とは違い、「そのテストをピンポイントで指定した場合はテストが実行される」ので、Ignore にしておくよりもこういう「しばらく Red のテスト」にはいいのかな。
Ignore は切り替えが面倒だけど、Explicit は自分でコントロールできるので。

時間のテスト

時間関係のテストはFakeを使って、テストをやりやすくする

TDD Boot Campに参加しました - @ikikko のはてなブログ

時間というものをテストするということで頭の中に警鐘が鳴り響きました。
レガシーコード改善ガイドにおける、「単体テスト」、すなわち早いテストをすることが難しいからです。
〜略〜
ではどうするのか。
頭の中で数分考えたのち、ブログで前にほとんど同じことを教えてもらったことがあった*13ことを思い出しました。


*13: 参考：　http://d.hatena.ne.jp/YokoKen/20081027/1225071710

TDD Boot Camp体験記 - Logic Dice

時間とテスト - Logic Dice
の当たりの話ですね。
DateTime.Now とか便利だけど考えさせられるというか・・・

TDD と 開発環境

  • 環境周りを整備すればよかった
    • 最低、リポジトリ用意
    • もっと言えば、CIも用意
      • CIとの連携を手軽にやるならMavenになるけど、知らない人にはハードル高い
      • 演習のレベルではあまりCIのメリットがないかも*2


*2: プロダクトクラス・テストクラスそれぞれ1つずつだから、コンパイルエラーやテスト漏れもまず起きないだろうし

TDD Boot Campに参加しました - @ikikko のはてなブログ

Git と Hudson 連携させると超便利！
ただ、コミットのタイミングはちょっと迷う。

  • 固定値を返すだけの実装を通した後でコミットする
  • それを失敗させるようなテストを書いた上で、それを Green にしてからコミットする

うーん、まぁ 2 つ目かなぁ。
Git の場合は 1 つ目の段階でもコミットしておいて、あとで git rebase -i origin で squash してしまえるから便利！

入出力の網羅性

  • 自分を含め、初心者はテストコードを沢山書いてしまう。
    • 明らかにGREENになるのがわかってるのに書いてしまう。
    • たぶん、これは通過儀礼。

素早くテンポ良く回すということを心がけていくので、特にテストの組み合わせについて網羅性を重要視する必要性はあまりありません*8。むしろ、自分の間違いやすい癖を見つけ、そこを重点的にカバーしていくなど、ある程度自分の経験に基づいた判断をしてもいいという話があったと思います。


*8：個人的には、このあたりも作成するクラスのコンテキストに依存するかなと思う。また、TDDのテストとは別にテストを作ってもいいわけだから、厳密なテストと軽快なテストに分けてもよいと思う。ただし、厳密なテストが軽快であるなら、それをTDDのテスト、すなわち繰り返し実行されるテストとして後から組み込んでもよいのではないだろうか

http://d.hatena.ne.jp/a-hisame/20091220/1261342174

単体テストやユニットテストと聞くと、どうしても入出力を網羅したテストを考えがちだけど、そうじゃない、そうじゃないんだ。
ということ。
ここら辺は名古屋 Ruby 会議 01 で id:t-wada (和田さん) と色々と話したときも話題に上ったんだけど、「テスト」という語感からどうしても「品質保証のためのテスト」を思い浮かべてしまう、というのがあると思う。
そうじゃなくて、TDD のテストは「開発者のためのテスト」であるという考えが重要だと思う。


そこでテストという名前と今までの道具をあえて捨てて、新しい言葉、新しい道具で TDD を再スタートさせたのが BDD・・・という理解。
つまり、TDD も BDD も同じものだよ！という。


でも、「通過儀礼」という考え方は無かったなぁ。
入出力を網羅したようなテストを重視する TDD は、完全に TDD とは別物と考えてたんだけど、そうか、そう言う考え方もあるのか。
問題は、それを正しい方向に向かわせてくれる人に出会えるかどうか、ですね。
そう言う意味で (参加してないけど) こういうイベントがもっとあるといいですね！


・・・と、ここまでが (参加してないけど) イベントの感想とかです。
で、以下は TDD に対する疑問。

TDD で言語に用意された assert って書くの？書くとしたらいつ？

つまり、DbC との兼ね合いはどうなるの？ってこと。
兼ね合わせる必要がそもそもあるのかどうか、あるとしたらどうやって兼ね合わせるのか・・・

ドキュメンテーションコメントは書くの？書くとしたらいつ？

疑問です・・・
書くとしたらリファクタリング後、ってことになるのかなぁ・・・

最後に

次あったら是非行きたいなぁ・・・

*1:なんだかこのあと修羅場が続くようなんだけど、鍋に誘った

コンストラクタで final なフィールドをあきらめない方法

Java

思いつきエントリ。後で説明とか付け加える予定。付け加えた。


final なフィールドは基本的にコンストラクタ内部で初期化することしか出来ない。
でも、そのフィールドを初期化する方法が複雑な場合、素直に実装するとコンストラクタがどんどんふくれあがってしまう。
なのでメソッドに分割したい・・・というのはまぁ普通によくあることなんだけど、例えそのメソッドがコンストラクタからしか呼び出されていなかったとしても、

// コンパイルエラーになる
public final class Hoge {
    final int hoge;
    public Hoge(int piyo) {
        prepareHoge(piyo);
    }
    // コンストラクタからしか呼び出されない
    private void prepareHoge(int piyo) {
        // 何かとても複雑な処理
        // ...
        hoge = result;
    }
}

こういうコードはコンパイルを通らない。

private static なメソッドを使う方法

上で示したプログラムはコンパイルを通らないが、それはコンストラクタ内部でフィールドを初期化していないからだ。
なので、prepareHoge メソッドの戻り値として、複雑な処理をした結果を返し、それをコンストラクタ内で hoge の初期化に使えばいい。

public final class Hoge {
    final int hoge;
    public Hoge(int piyo) {
        hoge = prepareHoge(piyo);
    }
    private static int prepareHoge(int piyo) {
        // 何かとても複雑な処理
        // ...
        return result;
    }
}

ここでは、prepareHoge の戻り値の型を変更しただけではなく、static に変更している。
非 static だと Hoge クラスのフィールドにアクセスすることが出来るが、このメソッドがコンストラクタから呼び出されることを考えると、prepareHoge が使う入力はすべて引数で渡すべきという判断から static にしている。
こうすることで、まだ初期化されていないフィールドにアクセスしてしまうことがなくなる。

// 非staticの場合
public final class Hoge {
    final int hoge;
    final int piyo;
    public Hoge() {
        // ここをみただけではおかしいことに気付かない
        hoge = prepareHoge();
        piyo = preparePiyo();
    }
    private int prepareHoge() {
        // ここをみただけでもpiyoが先に初期化されているはずという先入観などがあると気付きにくい
        return piyo + 10;
    }
    private int preparePiyo() {
        return 32;
    }
    
    public static void main(String[] args) {
        Hoge h = new Hoge();
        System.out.println(h.hoge); // 42・・・ではなく、10
        System.out.println(h.piyo); // 32
    }
}

// staticの場合
public final class Hoge {
    final int hoge;
    final int piyo;
    public Hoge() {
        // コンパイルエラーが出てくれる
        hoge = prepareHoge(piyo);
        piyo = preparePiyo();
    }
    private static int prepareHoge(int piyo) {
        return piyo + 10;
    }
    private static int preparePiyo() {
        return 32;
    }
}

初期化用のクラスを分ける方法

ただ、上記の方法だけでは問題がある場合もある。
例えば、設定ファイルを読み込んで各種フィールドを設定するような場合だ。

public final class Hoge {
    final int hoge;
    final int piyo;
    public Hoge(String configFilePath) {
        // 何回もファイルを読み込むのは微妙
        hoge = prepareHoge(configFilePath);
        piyo = preparePiyo(configFilePath);
    }
    private static int prepareHoge(String configFilePath) {
        // ファイルからhogeを取り出す
    }
    private static int preparePiyo(String configFilePath) {
        // ファイルからpiyoを取り出す
    }
}

さすがに、何回も設定ファイルを読み込むのは避けたい。
これを解決するには、クラス内部からのみ使用される単純なクラスを用意するという方法がある。

public final class Hoge {
    final int hoge;
    final int piyo;
    // loadの結果をprivateなコンストラクタに丸投げ
    public Hoge(String configFilePath) {
        this(load(configFilePath));
    }
    private Hoge(HogeData data) {
        hoge = data.hoge;
        piyo = data.piyo;
    }
    private static HogeData load(String configFilePath) {
        HogeData result = new HogeData();
        // 設定ファイルからresult.hogeとresult.piyoのデータを読み込む
        return result;
    }
    // 初期化のみに使用するクラス
    // private staticなクラスなので、外部から使用されることはない。
    // そのため、このクラスのフィールドをfinalにする必要性はない。
    private static final class HogeData {
        int hoge;
        int piyo;
    }
}

このように、初期化のみに使用するクラスのオブジェクトを受け取る private なコンストラクタを用意し、public なコンストラクタは処理を丸投げしてしまう。
設定ファイルの読み込み処理は、初期化のみに使用するクラスを返す private なメソッドを用意することで解決している。


クラスのかわりに Map を使うだとかも出来るんだけど、まぁそれはそれ。

asList のシグニチャとジェネリクス

Java

asList に関しては前にも苦言を呈しているんですが・・・

返されるリストは直列化可能で、RandomAccess を実装します。

Oracle Technology Network for Java Developers

とか書いてあるんですよねー。
なら、シグニチャは

public static <T> List<T> asList(T... a)

じゃなくて、

public static <T, L extends List<T> & Serializable & RandomAccess>
L asList(T... a)

の方がいいんじゃないのかな・・・
いや、ただそれだけ。

final について？

Java OOP

final 周辺について。理想論？いや、理想論大事。

OCP (Open-Closed Principle)

開放閉鎖原則とも。
簡単に言うと、モジュール (ここでは class) は拡張できるべきだが、修正は行うべきではない、という原則。
これを原則に従うと、(もっと一般的な意味での) 修正が容易になる。
「拡張できるべき」と「修正は行うべきではない」を両立しないといけないので、一見、継承はこの原則を守るためには使っても良さそうなものだけど・・・

実装の継承が OCP を破る例

例えば、

class Rectangle {
    int w;
    int h;
    Rectangle(int w, int h) {
        this.w = w;
        this.h = h;
    }
    
    int width() { return w; }
    int height() { return h; }
    
    void setWidth(int newW) { w = newW; }
    void setHeight(int newH) { h = newH; }
}

こんなクラスがあったとする。
で、正方形が欲しくなったとして、Rectangle から継承したとする。

class Square extends Rectangle {
    Square(int l) {
        super(l, l);
    }
    
    @Override
    void setWidth(int newL) { w = h = newL; }
    
    @Override
    void setHeight(int newL) { w = h = newL; }
}

こんな感じ・・・なのだが、例えば以下のようなメソッドが既存コード中に存在したとする。

void method(Rectangle rect) {
    rect.setWidth(20);
    rect.setHeight(10);
    
    assert rect.width() != rect.height();
}

このコードには、Rectangle はもちろん、Square のインスタンスも渡すことが出来てしまうが、アサーションで引っかかってしまう。
つまり、既存コードに修正が必要となる。

LSP (Liskov Substitution Principle)

リスコフの置換原則とも。
簡単に言うと、サブクラスはスーパークラスと同じように使えなければならない、という原則。
上の例は、実はこちらの原則を破っているために OCP 違反にもなっている例になっている。
LSP に違反すると、OCP にも違反することになるため、こちらの原則を満たせないような「拡張性」は、何かしら既存コードを「修正」する必要性がある、ということになる*1。
ではどうすれば LSP を守れるのか。

DbC (Design by Contract)

契約による設計とも。
LSP はこの DbC とつながりがあり、以下の条件を満たすようにメソッドをオーバーライドすれば、LSP を守ることが出来る。

  • 事前条件はスーパークラスの事前条件と同じにするか、弱い条件と置き換える
  • 事後条件はスーパークラスの事後条件と同じにするか、強い条件と置き換える

実際的な例を出すと、オーバーライドするメソッドでは、

  • より広い入力を受け付ける (より広い定義域)
  • より抽象的な型を受け付ける
  • より狭い出力を返す (より狭い値域)
  • より具体的な型を返す

ようにする、などなど。

でも・・・

Java で事前条件はともかく、事後条件って書きにくいよね・・・

そこで final！

final 付けとけば LSP 守るの簡単だよ！
ってことで上の例はどうすれば良かったのか？の一例。

public interface Rectangle {
    int width();
    int height();
    void setWidth(int newW);
    void setHeight(int newH);
}

final class ResizableRectangle implements Rectangle {
    int w;
    int h;
    ResizableRectangle(int w, int h) {
        this.w = w;
        this.h = h;
    }
    
    public int width() { return w; }
    public int height() { return h; }
    
    public void setWidth(int newW) { w = newW; }
    public void setHeight(int newH) { h = newH; }
}

void method(Rectangle rect) {
    rect.setWidth(20);
    rect.setHeight(10);
    
    assert rect.width() != rect.height();
}

このクラスは、継承を final により禁止しているので、LSP はもちろん破っていない。というか破りようがない。
また、既存コードを変更することなく、Square クラスを追加するという形で拡張することが出来るため、OCP 違反にもなっていない。

問題になるところ

いいことばかりではない。
例えば、上の例では ResizableRectangle は final なので継承できない。そら、LSP と OCP 守るために final 付けたんだから継承できるわけないんだけど・・・
ResizableRectangle で何か超複雑なことをしていて、その機能が欲しい・・・でも、ResizableRectangle のソースはない。
こういう状況だったら委譲を使うことになるだろうけど、Java で委譲・・・いくら IDE のサポートがあるとはいえ、何とかならんものか・・・
ResizableRectangle が final でさえなければ、継承という手っ取り早い方法が使えるのだけれども・・・

結局 final は付けるべき？付けないべき？

それは、まぁ、場合によるよね、としか。
final を付けるか付けないか迷うと言うことは、そのクラス設計がおかしいのではないか？とか言ってみる。
final を外したいという衝動が「流動的な要素をなにか見逃していないか」という不安から来ているのだとすれば、もう少しクラスを分割してみるとか。
メソッドの引数として受け取るクラスが final でどうしようもない・・・と言うときは、より抽象的なクラスをメソッドの引数にすべきではなかったのかとか*2。


final を多用したとしても、出来る限りそれを苦に思わせないようなやり方、ってのはあると思うんです。
現在あるライブラリ *3 で、final に苦しめられたからと言って、無条件で「final 付けるな」というのは、無条件で「final 付けろ」というのと変わらないんじゃないですかね。
委譲が面倒って、それ SRP *4 破りまくってるからじゃないですかね。
つか、package private なクラスだったら final 付けなくても final のようなもんなんですよね。class はすべて何らかの interface を実装しなければならない、という極論を言うつもりはないんですけど、public な部分に関してはこれ言っちゃっていいんじゃないだろうか・・・*5 *6
・・・と、半ば本気で思っていたり。夢見がちな年頃なのです。


以下追記

ここら辺の話は何読めばいいの？

とりあえず、この 2 冊。

オブジェクト指向入門 第2版 原則・コンセプト (IT Architect’Archive クラシックモダン・コンピューティング)

オブジェクト指向入門 第2版 原則・コンセプト (IT Architect’Archive クラシックモダン・コンピューティング)


アジャイルソフトウェア開発の奥義 第2版 オブジェクト指向開発の神髄と匠の技

アジャイルソフトウェア開発の奥義 第2版 オブジェクト指向開発の神髄と匠の技

あと流動的要素とかその辺の話。

デザインパターンとともに学ぶオブジェクト指向のこころ (Software patterns series)

デザインパターンとともに学ぶオブジェクト指向のこころ (Software patterns series)

*1:もちろん、上で例示した method の様なコードがなければ問題は表面化しない。しかし、逆に言うと上のようなメソッドを追加してしまった場合、修正が必要となるので結局 OCP を破っている

*2:これを推し進めると、public な API では final な具象型を引数に取らいとか、そういう方向に進むことに

*3:標準のものを含む

*4:Single Responsibility Principle：単一責任原則

*5:値クラスは別かな・・・とは言っても、値クラスは値クラスで、Comparable とか Serializable とか実装してる可能性ががが

*6:ユーティリティクラスは別か。そもそもユーティリティクラスがあまり好きではないのだけど、それはまた別の話

もう少し柔軟な assertThat が欲しい

Java

assertThat と hamcrest の組み合わせって便利ですよねー。
assertThat と hamcrest の組み合わせについては、
JDaveの寄り道にhamcrestを試してみる。 - Fight the Future
とか、
hamcrestのMatcherメモ - 都元ダイスケ IT-PRESS
とか見てもらえば大体分かると思う。


問題は、assertThat メソッド。

このメソッド、シグニチャが

public static <T> void assertThat(T actual, Matcher<T> matcher);

な感じになっている。
なので、

class Hoge { ... }
class ExHoge extends Hoge { ... }

Hoge hoge() { return new ExHoge(); }

なんてクラスとメソッドがあった場合に、

assertThat(hoge(), is(new ExHoge()));

なんてしてしまうと、コンパイルエラーになってしまう*1。
こんな風にキャストすればいいんだけど、

assertThat(hoge(), is((Hoge) new ExHoge()));

激しください・・・


actual 側、つまり実際のコードでは抽象化されたものを使うけど、その assert にまで抽象化されたものを要求するのはちょっと微妙じゃないだろうか？
ってことで、こんなメソッドを作って、static import して使ってみた。

public static <T> void assertThat(T actual, final Matcher<? extends T> matcher) {
    org.junit.Assert.assertThat(actual, new BaseMatcher<T>() {
        @Override
        public boolean matches(Object obj) {
            return matcher.matches(obj);
        }
        @Override
        public void describeTo(Description description) {
            matcher.describeTo(description);
        }
    });
}

これで

assertThat(hoge(), is(new ExHoge()));

と書ける！
・・・んー、んー、なんかこんなこと書かなくても実現できそうな気もするんだけど・・・独自の Matcher 返す is メソッド書くとかなると、型パラメータが問題だし・・・
求む、もっといい感じの解決策！

*1:actual の型は Hoge、matcher の型 は Matcher となり、T が一致しない