アジャイルソフトウェア開発の奥義

本 OO Java

アジャイルソフトウェア開発の奥義

アジャイルソフトウェア開発の奥義

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

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

会社の勉強会でこの本を読んでいます。
書評を見ると評判いいみたいだけれど、サンプルコードがイマイチなのが気になった。
いくらいいことを言っても結果がダメだと説得力が半減してしまう。

素数の生成 (第5章)

「エラトステネスのふるい」で素数を生成するプログラム。
最初のバージョンは良かったのに、リファクタリングして、一時変数を static フィールドに置いたため、複数のスレッドから generatePrimes() を実行するとエラーが発生する可能性がある。

public class PrimeGenerator
{
  private static boolean[] crossedOut;
  private static int[] result;

  public static int[] generatePrimes(int maxValue)
  {
    if (maxValue < 2)
      return new int[0];
    else
    {
      uncrossIntegersUpTo(maxValue);
      crossOutMultiples();
      putUncrossedIntegersIntoResult();
      return result;
    }
  }

  private static void uncrossIntegersUpTo(int maxValue)
  {
    crossedOut = new boolean[maxValue + 1];
    for (int i = 2; i < crossedOut.length; i++)
      crossedOut[i] = false;
  }

  // 以下省略。
}

こんなテストケースを追加すると、問題が発覚する。

public void testPrimesConcurrently() throws Exception {
    Runnable task = new Runnable() {
        public void run() { testPrimes(); }
    };
    List<Future<?>> futures = new ArrayList<Future<?>>();
    ExecutorService executor = Executors.newCachedThreadPool();
    for (int i = 0; i < 100; ++i) {
        futures.add(executor.submit(task));
    }
    for (Future<?> future : futures) {
        future.get(10, TimeUnit.SECONDS);
    }
}

public void testPrimes() {
  // 一部省略。

    int[] centArray = PrimeGenerator.generatePrimes(100);
    assertEquals(centArray.length, 25);
    assertEquals(centArray[24], 97);
}

今回はシングルスレッドでしか使わないと言うかもしれないけれど、並行性のバグは思わぬところで発生して性質が悪いので、日頃からこういうコードは書くべきではないと思う。
とりあえず動くようにするには、一時変数と各メソッドの static をはずして、generatePrimes() を実行するたびに PrimeGenerator のインスタンスを生成するように修正すればよい。

public class PrimeGenerator
{
  private boolean[] crossedOut;
  private int[] result;

  public static int[] generatePrimes(int maxValue)
  {
    if (maxValue < 2)
      return new int[0];
    else
    {
      PrimeGenerator generator = new PrimeGenerator();
      generator.uncrossIntegersUpTo(maxValue);
      generator.crossOutMultiples();
      generator.putUncrossedIntegersIntoResult();
      return generator.result;
    }
  }

  private void uncrossIntegersUpTo(int maxValue)
  {
    crossedOut = new boolean[maxValue + 1];
    for (int i = 2; i < crossedOut.length; i++)
      crossedOut[i] = false;
  }

  // 以下省略。
}

ということで、

一時変数を static なフィールドに置いてはいけない。

あと蛇足だけれど、郷に入れば郷に従えで、Java の標準的なコーディング規約に従おうよ。
assertEquals() の引数の順番も逆だし。

ボウリングゲーム (第6章)

これも、一番の問題は変数のスコープ。ボウリングの「倒したピンの記録」と「スコアの計算で使う一時変数」を同じクラスのフィールドに置いてしまったため、Game#score() を並行に実行することができない。

public class Game
{
  public int score()
  {
    return scoreForFrame(itsCurrentFrame);
  }

  public void add(int pins)
  {
    itsScorer.addThrow(pins);
    adjustCurrentFrame(pins);
  }

  // 一部省略。

  public int scoreForFrame(int theFrame)
  {
    return itsScorer.scoreForFrame(theFrame);
  }

  private int itsCurrentFrame = 0;
  private boolean firstThrowInFrame = true;
  private Scorer itsScorer = new Scorer();
}

public class Scorer
{
  public void addThrow(int pins)
  {
    itsThrows[itsCurrentThrow++] = pins;
  }

  public int scoreForFrame(int theFrame)
  {
    ball = 0;
    int score=0;
    for (int currentFrame = 0;
         currentFrame < theFrame;
         currentFrame++)
    {
      if (strike())
      {
        score += 10 + nextTwoBallsForStrike();
        ball++;
      }
      else if ( spare() )
      {
        score += 10 + nextBallForSpare();
        ball+=2;
      }
      else
      {
        score += twoBallsInFrame();
        ball+=2;
      }
    }

    return score;
  }

  // 一部省略。

  private int ball;
  private int[] itsThrows = new int[21];
  private int itsCurrentThrow = 0;
}

先程と同様に、こんなテストケースを追加すると、問題が発覚する。
この章では assertEquals() の引数の順番はあってますね。

public void testPerfectGameConcurrently() throws Exception {
    for (int i=0; i<12; i++) {
        g.add(10);
    }
    Runnable task = new Runnable() {
        public void run() {
        assertEquals(300, g.score());
      }
    };
    List<Future<?>> futures = new ArrayList<Future<?>>();
    ExecutorService executor = Executors.newCachedThreadPool();
    for (int i = 0; i < 1000; ++i) {
        futures.add(executor.submit(task));
    }
    for (Future<?> future : futures) {
        future.get(10, TimeUnit.SECONDS);
    }
}

とりあえず動くようにするなら、「倒したピンの記録」を Game に移動して、スコアを計算するたびに Scorer のインスタンスを生成するのが簡単かな?

public class Game
{
  public int score()
  {
    return scoreForFrame(itsCurrentFrame);
  }

  public void add(int pins)
  {
    addThrow(pins);
    adjustCurrentFrame(pins);
  }

  private void addThrow(int pins)
  {
    itsThrows[itsCurrentThrow++] = pins;
  }

  // 一部省略。

  public int scoreForFrame(int theFrame)
  {
    Scorer scorer = new Scorer(itsThrows);
    return scorer.scoreForFrame(theFrame);
  }

  private int[] itsThrows = new int[21];
  private int itsCurrentThrow = 0;
  private int itsCurrentFrame = 0;
  private boolean firstThrowInFrame = true;
}

public class Scorer
{
  public Scorer(int[] throws_)
  {
    itsThrows = throws_;
  }

  public int scoreForFrame(int theFrame)
  {
    // 変更なし。
  }

  // 一部省略。

  private int ball;
  private int[] itsThrows;
}

でも、せっかくセッションでこんなクラス図
http://www.kodougu.net/u/glad2121/diagram/269.png
を描いたり、

Frame は自分自身の前後にある他の Frame へのポインタを保持していればいい。Frame のスコアを計算するときには、まず1つ前の Frame のスコアを取得して、もしスペアやストライクを取っている場合は1つ後の Frame の必要な部分を取得すればいい。

のようないいアイデアを出しているのに、全然活かされていない。

すべてのプログラムでオブジェクト指向設計を使わなければならないのだろうか? ここで取り上げたケースは、単にプログラムにその必要性がなかっただけだ。

なんて言いわけしてるけれど、Scorer#scoreForFrame() のように Frame の概念が曖昧になっているより、Frame クラスを抽出した方が、僕はわかりやすいコードになると思う。

で、書いてみたコードは以下のとおり。

public class Game {

    private final Frame[] frames = new Frame[10];

    private int currentFrame = 0;

    public Game() {
        Frame frame = new FirstFrame();
        frames[0] = frame;
        for (int i = 1; i < 9; ++i) {
            frame = new Frame(frame);
            frames[i] = frame;
        }
        frames[9] = new LastFrame(frame);
    }

    public int score() {
        return scoreForFrame(currentFrame);
    }

    public int scoreForFrame(int frame) {
        if (frame == 0) {
            return 0;
        }
        return frame(frame - 1).scoreForFrame();
    }

    public void add(int pins) {
        Frame frame = frame(currentFrame);
        frame.addThrow(pins);
        if (frame.wasLastThrowInFrame()) {
            ++currentFrame;
        }
    }

    private Frame frame(int frame) {
        return frames[frame];
    }

}

public class Frame {

    protected final int[] throws_;
    protected final Frame prevFrame;
    protected Frame nextFrame;

    protected int currentThrow = 0;

    public Frame(Frame prevFrame) {
        this(2, prevFrame);
    }

    protected Frame(int count, Frame prevFrame) {
        this.throws_ = new int[count];
        this.prevFrame = prevFrame;
        if (prevFrame != null) {
            prevFrame.nextFrame = this;
        }
    }

    public void addThrow(int pins) {
        throws_[currentThrow++] = pins;
    }

    public boolean wasLastThrowInFrame() {
        return (currentThrow == 2) || isStrike();
    }

    public int scoreForFrame() {
        int score = scoreForPrevFrame();
        if (isStrike()) {
            score += (10 + nextTwoBallsForStrike());
        } else if (isSpare()) {
            score += (10 + nextBallForSpare());
        } else {
            score += twoBallsInFrame();
        }
        return score;
    }

    protected int scoreForPrevFrame() {
        return prevFrame.scoreForFrame();
    }

    protected boolean isStrike() {
        return pins(0) == 10;
    }

    protected boolean isSpare() {
        return twoBallsInFrame() == 10;
    }

    protected int pins(int ball) {
        return throws_[ball];
    }

    protected int nextTwoBallsForStrike() {
        return nextFrame.firstTwoBallsForStrike();
    }

    protected int nextBallForSpare() {
        return nextFrame.pins(0);
    }

    protected int twoBallsInFrame() {
        return pins(0) + pins(1);
    }

    protected int firstTwoBallsForStrike() {
        if (!isStrike()) {
            return twoBallsInFrame();
        } else {
            return pins(0) + nextFrame.pins(0);
        }
    }

}

public class FirstFrame extends Frame {

    public FirstFrame() {
        super(null);
    }

    @Override
    protected int scoreForPrevFrame() {
        return 0;
    }

}

public class LastFrame extends Frame {

    public LastFrame(Frame prevFrame) {
        super(3, prevFrame);
    }

    @Override
    public boolean wasLastThrowInFrame() {
        return (currentThrow == 3) ||
                (currentThrow == 2 && !isStrike() && !isSpare());
    }

    @Override
    protected int nextTwoBallsForStrike() {
        return pins(1) + pins(2);
    }

    @Override
    protected int nextBallForSpare() {
        return pins(2);
    }

    @Override
    protected int firstTwoBallsForStrike() {
        return twoBallsInFrame();
    }

}

FirstFrame、LastFrame は、クラスを分けずに if 文にしちゃってもいいと思うけどね (最初のバージョンはそうしてた)。

クラス図にすると、こんな感じ。
http://www.kodougu.net/u/glad2121/diagram/270.png
みなさんは、どちらの方がわかりやすいと思いますか?

まとめると、

状態変数と一時変数を同じスコープに置いてはいけない。
オブジェクト指向で、仕様を素直に実装しよう。

長方形と正方形 (第9・10章)

最後は、コードの批判ではなくて、この本を読んで気づいたこと。

数学的に「正方形は長方形の一種」だからといって、「長方形」クラスを継承して「正方形」クラスを作ってはいけない。

と、よく言われるけれど、今まで自分自身その理由をちゃんと説明できていたかというと、ちょっと怪しかったかもしれない。
では、なぜ継承してはいけないのかというと、長方形に「縦と横の長さを独立して変更できる」という振る舞いがあった場合、継承して正方形を作るとリスコフの置換原則に違反してしまうからだ。
ということは、「縦と横の長さが不変」であれば、長方形を継承して正方形を作っても良いのではないか?
http://www.kodougu.net/u/glad2121/diagram/271.png
moveTo() とか、相似変換、色の変更などはあってもかまわない。ですよね?

謝辞

  • クラス図の作成には Kodougu を使わせていただきました。m(_ _)m