プログラムは語る: toString編1

新人Tさんに「コレクションを与えると各要素を文字列化したものを”, “で連結した文字列を返す」メソッドを作ってもらいました。数分で提出してくれると思っていましたが、彼には難しかったようです。オブジェクト指向とJavaを勉強し始めて間もないので仕方がないのかもしれません。

次のプログラムは彼が提出してくれたもの（を説明用に修正したもの）です。このプログラムからはいろいろなことが読み取れます。まずは見てください。

■プログラム（Tさん作）

import java.util.Iterator;
import java.util.Collection;
public class CollectionUtility
{
    private Object mBuffer;
    private String mString;
    public String toString(Collection aCollection)
    {
        Iterator tIterator = aCollection.iterator();
        for(int i = 0; i < aCollection.size(); i++)
        {
            if(i == 0)
            {
                mBuffer = tIterator.next();
                mString = mBuffer.toString();
            }
            else
            {
                mBuffer = tIterator.next();
                mString += ", " + mBuffer.toString();
            }
        }
        return mString;
    }
}

どうですか？ものすごく突っ込みどころ満載ですよね。突っ込みどころがありすぎて、どこから突っ込んでいいのか悩んでしまいます。（突っ込みどころが分からない人は職業プログラマとして相当やばいですよ。）とりあえず、すぐに分かるところから指摘してゆきます。

その変数は本当にフィールドでなければいけないのか？

まず、多くの人が突っ込みたくなる点といえばこれでしょう。フィールドmBufferとmStringはメソッドtoString(Collection)の中で使う情報を保持するために使われています。これらはフィールドとして宣言しなければいけないのでしょうか？

答えは「いいえ」です。これらはローカル変数として宣言すべきです。これらが保持する情報にのその他からアクセスできる必要がないからです。必要がないのにアクセスできてしまうと次のような保守上の問題が発生します。

• 誤ってアクセス（参照・更新）してしまう。
• 把握しなければいけない仕様が増える。

また、性能上の観点から考えることもできます。

実行環境にもよりますが、一般にフィールドアクセス時間はローカル変数アクセス時間の2倍程度です。つまり、それだけ速度が犠牲になります。

同じフィールドを更新するメソッドを同時に呼び出すと排他制御なしでは正しく動作しません。もし排他制御を行わないなら、インスタンスを分けて更新するフィールドが同じにならないようにする必要があります。どちらにせよ、速度かメモリ量が犠牲になります。

これらの理由から、性能を犠牲にしないですむようにローカル変数として宣言するべきです。

以上のことを踏まえた上で、Tさんに修正してもらいました。次のプログラムは修正後のもの（を説明用に修正したもの）です。

■プログラム（ローカル変数化＋staticメソッド化）

import java.util.Iterator;
import java.util.Collection;
public class CollectionUtility
{
    public static String toString(Collection aCollection)
    {
        Object tBuffer;
        String tString = "";
        Iterator tIterator = aCollection.iterator();
        for(int i = 0; i < aCollection.size(); i++)
        {
            if(i == 0)
            {
                tBuffer = tIterator.next();
                tString += tBuffer;
            }
            else
            {
                tBuffer = tIterator.next();
                tString += ", " + tBuffer;
            }
        }
        return tString;
    }
}

このプログラムでは、フィールmBufferとmStringドをローカル変数tBufferとtStringにしてるだけではなく、メソッドtoString(Collection)がフィールドに依存しなくなったのでそれをstaticにしてあります。こうすることでメソッドtoString(Collection)を呼び出すためにクラスCollectionUtiltiyをインスタンス化する必要がなくなりました。

このプログラムにはもう2つの修正があります。

1つ目はローカル変数tStringに空文字列””を初期値として与える修正です。フィールドは参照する前に初期化しなくてもコンパイラに警告してもらえませんが、ローカル変数は警告してもらえます。このコンパイラの警告によって「引数aCollectionの要素数が0である場合、最後にmStringに代入された値が返される」というバグが発見できました。ローカル変数を使うということはこのような効果もある訳です。

2つ目はローカル変数tBufferを明示的に文字列化せずに連結する修正です。修正前はローカル変数tBufferにnullが代入される（引数aCollectionの要素にnullがある）場合に例外NullPointerExceptionが送信され異常終了してしまいます。この修正はそのような場合にも戻り値を返すようにする工夫です。

とりあえず、少しましなプログラムになりましたが、まだまだ突っ込みどころ満載です。そういう訳で、次回もこのプログラムを修正します。