自分で書いたレガシーコードを自分で改善する

はじめに

リンクアンドモチベーションでソフトウェアエンジニアをしているkoboriです。
つい最近、研修でコード改善の講義を受けたので、その復習も兼ね、なるべく読みにくいレガシーコードを自分で書き、自分で改善をして、その過程を記事にすることにしました。

レガシーコードを用意する

まず初めになるべく読みにくいレガシーコードを作成していきます。ここでいう レガシーコードとは、 テストがないコードを指しています。
ただ、自分で書いたテストがないコードを自分で改善するのでは、少し改善の難易度が低いと思ったので、下記の4点に留意して可読性を大幅に下げました。

• テストは絶対に書かない
• 変数はなるべく短く、省略する
• マジックナンバーを積極的に多用する
• クラスとメソッドはなるべく大きくする
• このコードを書いてから改善するまで3日以上の間隔を開ける

その結果書かれたコードがこちらです。

minesweeper.rb

class Minesweeper
    def self.start
        x1 = [{ d: "■", n: 0, m: false, f: false }, { d: "■", n: 0, m: false, f: false }, { d: "■", n: 0, m: false, f: false }]
        x2 = [{ d: "■", n: 0, m: false, f: false }, { d: "■", n: 0, m: false, f: false }, { d: "■", n: 0, m: false, f: false }]
        x3 = [{ d: "■", n: 0, m: false, f: false }, { d: "■", n: 0, m: false, f: false }, { d: "■", n: 0, m: false, f: false }]
        b = [x1, x2, x3]

        mine1 = [rand(0..2), rand(0..2)]
        mine2 = [rand(0..2), rand(0..2)]
        loop do
            break if mine1 != mine2
            mine2 = [rand(0..2), rand(0..2)]
        end
        b[mine1[0]][mine1[1]][:m] = true
        b[mine2[0]][mine2[1]][:m] = true

        (0..2).each do |i|
            (0..2).each do |j|
                if b[i][j][:m] == false
                    top = i - 1
                    down = i + 1
                    left = j - 1
                    right = j + 1
                    if top >= 0
                        b[i][j][:n] += 1 if b[top][j][:m] == true
                    end
                    if down <= 2
                        b[i][j][:n] += 1 if b[down][j][:m] == true
                    end
                    if left >= 0
                        b[i][j][:n] += 1 if b[i][left][:m] == true
                    end
                    if right <= 2
                        b[i][j][:n] += 1 if b[i][right][:m] == true
                    end
                    top_left = [top, left]
                    top_right = [top, right]
                    down_left = [down, left]
                    down_right = [down, right]
                    if top_left[0] >= 0 && top_left[1] >= 0
                        b[i][j][:n] += 1 if b[top_left[0]][top_left[1]][:m] == true
                    end
                    if top_right[0] >= 0 && top_right[1] <= 2
                        b[i][j][:n] += 1 if b[top_right[0]][top_right[1]][:m] == true
                    end
                    if down_left[0] <= 2 && down_left[1] >= 0
                        b[i][j][:n] += 1 if b[down_left[0]][down_left[1]][:m] == true
                    end
                    if down_right[0] <= 2 && down_right[1] <= 2
                        b[i][j][:n] += 1 if b[down_right[0]][down_right[1]][:m] == true
                    end
                end
            end
        end
        
        puts "マインスイーパを開始します。"
        puts "爆弾は全部で#{2}個あります。"
        puts "回答を入力してください。(例: 1,1)"
        puts b.map { |x| x.map { |s| s[:d] }.join(" ") }

        loop do
            line = gets.split(',')
            x = line[0].to_i
            y = line[1].to_i
            if b[x-1][y-1][:m] == true
                puts "爆弾です。"
                break 
            end
            b[x-1][y-1][:d] = b[x-1][y-1][:n]

            cl = b.all? do |x|
                x.all? do |h|
                    h[:d] != "■" || h[:m] == true
                end
            end

            if cl == true
                puts "---"
                puts "クリアです。"
                puts "---"
                puts b.map { |x| x.map { |s| s[:m] == true ? "✖" : s[:d] }.join(" ") }
                break
            end

            puts "---"
            puts "次の回答を入力してください。"
            puts "---"

            puts b.map { |x| x.map { |y| y[:d] }.join(" ") }
        end

        puts "マインスイーパを終了します。"
    end
end

どんなアプリケーションか

作成したアプリケーションの説明を簡単に書きます。
課題となるアプリケーションの内容は、CUIで遊べるマインスイーパーにしました。
主な仕様は下記の通りです。

• 地雷原のマスは3×3で固定
• 地雷の数は2個で固定
• 1,1 のような形式のカンマ区切りで入力し、オープンする地雷を決定する
• 地雷のマスをオープンすると、ゲームオーバーになる
• 地雷以外のマスを全てオープンすると、クリアになる

改善のゴールを決める

改善を始めるにあたり、今回のゴールを下記で定めました。

1. Minesweeperクラスを読める状態にする
2. 単体テストを書くことができる状態にする

改善スタート

課題となるレガシーコードを書いてから4日経ったので、改善を始めていきます。

今回の改善作業の全体感は下記の通りです。

1. 最低限の挙動を担保するテストを追加する
2. Misweeperクラスの処理を別クラスに切り出す

これらの作業の過程で、変数名の修正等もやっていきます。

①最低限の挙動を担保するテストを追加する

まず初めにRspecを導入し、現在の挙動を担保する最低限のテストを追加します。
爆弾の位置と入力値をモックで固定化し、ゲームをクリアできた場合とクリアできなかった場合の2パターンのテストを追加しました。検査の対象は標準出力にしています。

それぞれのケースのテストコードは下記の通りです。

地雷を踏みゲームオーバーになった場合

    context 'クリアできなかった場合' do
      it '爆弾があったことを示す文字列が出力され、ゲームが中断される' do
        allow(Minesweeper).to receive(:rand).and_return(0, 0, 1, 1)

        allow(ARGF).to receive(:gets).and_return(StringIO.new('1,1').gets)

        expected_stdout = <<~EOS
  マインスイーパを開始します。
  爆弾は全部で2個あります。
  回答を入力してください。(例: 1,1)
  ■ ■ ■
  ■ ■ ■
  ■ ■ ■
  爆弾です。
  マインスイーパを終了します。
        EOS

        expect { Minesweeper.start }.to output(expected_stdout).to_stdout
      end
    end

地雷を踏まずゲームクリアできた場合

    context 'クリアできた場合' do
      it  'マスがすべて開き、正常にゲームが終了する' do
        allow(Minesweeper).to receive(:rand).and_return(0, 0, 1, 1)

        allow(ARGF).to receive(:gets).and_return(
          StringIO.new('1,2').gets,
          StringIO.new('1,3').gets,
          StringIO.new('2,1').gets,
          StringIO.new('2,3').gets,
          StringIO.new('3,1').gets,
          StringIO.new('3,2').gets,
          StringIO.new('3,3').gets,
        )

        expected_stdout = <<~EOS
  マインスイーパを開始します。
  爆弾は全部で2個あります。
  回答を入力してください。(例: 1,1)
  ■ ■ ■
  ■ ■ ■
  ■ ■ ■
  ---
  次の回答を入力してください。
  ---
  ■ 2 ■
  ■ ■ ■
  ■ ■ ■
  ---
  次の回答を入力してください。
  ---
  ■ 2 1
  ■ ■ ■
  ■ ■ ■
  ---
  次の回答を入力してください。
  ---
  ■ 2 1
  2 ■ ■
  ■ ■ ■
  ---
  次の回答を入力してください。
  ---
  ■ 2 1
  2 ■ 1
  ■ ■ ■
  ---
  次の回答を入力してください。
  ---
  ■ 2 1
  2 ■ 1
  1 ■ ■
  ---
  次の回答を入力してください。
  ---
  ■ 2 1
  2 ■ 1
  1 1 ■
  ---
  クリアです。
  ---
  ✖ 2 1
  2 ✖ 1
  1 1 1
  マインスイーパを終了します。
        EOS

        expect { Minesweeper.start }.to output(expected_stdout).to_stdout
      end
    end

追加したテストケースを実行し、無事通ることを確認できました。

②Misweeperクラスの処理を別クラスに切り出す

テストで足場を固め、レガシーコードと戦える体制が整ったので、Minesweeperクラスの改善に移ります。

テストの結果を頼りに、読みにくい変数名やキーの名前を修正していきつつ、Minesweeperクラスの処理を眺めていると、下記のように処理を分割できそうなポイントが見えてきました。

1. 盤面の初期状態を作る
2. 盤面を表示する
3. 盤面をオープンする
4. 地雷の判定をする
5. ゲームクリアの判定をする

その結果、盤面に対して操作をしている箇所が多いことが分かったため、Boardクラスを作成し、各メソッドのインターフェースを考えました。

board.rb

class Board
  attr_reader :squires

  def initialize(mine_count:, row_count:, column_count:)
    # 爆弾の数、盤面の縦と横の数を受け取り、盤面の初期状態を作る
  end

  def current_board
    # 現在のボードの状態を返す
  end

  def mine?(row_number:, column_number:)
    # 盤面の座標を受け取り、爆弾かどうかの判定をする
  end

  def open(row_number:, column_number:)
    # 盤面の座標を受け取り、オープンする
  end

  def completed?
    # クリア状態かどうかの判定をする
  end

  def completed_board
    # クリア後のボードの状態を返す
  end
end

あとは、Minesweeperクラスの処理を、1つずつテストを確認しながらBoardクラスへ切り出していき、ゲームにおける主要な処理の単体テストを書くことができる状態になりました。
余談ですが、利用者として ゲームの難易度を上げたかったため、爆弾と盤面のマス数を可変にする対応を、この過程で併せて実施しました。

詳細は下記に置いてあります。

改善後のMinesweeperクラス

命名を修正し、処理のまとまりに名前をつけて切り出す。この2つの作業でMinesweeperクラスをすっきりさせ、改善前と比べて読みやすいコードに修正することができました。
決してベストな状態であるとは言えないですが、今後の改善のスタートラインに立つことができたのではないかと思っています。

minesweeper.rb

require './board'

class Minesweeper
  MINE_COUNT = 2
  ROW_COUNT = 3
  COLUMN_COUNT = 3

  def self.start
    board = Board.new(
      mine_count: MINE_COUNT,
      row_count: ROW_COUNT,
      column_count: COLUMN_COUNT,
    )

    puts "マインスイーパを開始します。"
    puts "爆弾は全部で#{MINE_COUNT}個あります。"
    puts "回答を入力してください。(例: 1,1)"
    puts board.current_board

    loop do
      line = gets.split(',')
      row_input = line[0].to_i - 1
      column_input = line[1].to_i - 1
      if board.mine?(row_number: row_input, column_number: column_input)
        puts "爆弾です。"
        break 
      end
      board.open(row_number: row_input, column_number: column_input)

      if board.completed?
        puts "---"
        puts "クリアです。"
        puts "---"
        puts board.completed_board
        break
      end

      puts "---"
      puts "次の回答を入力してください。"
      puts "---"

      puts board.current_board
    end

    puts "マインスイーパを終了します。"
  end
end

さいごに

コードを改善する際には、修正作業の前にまず信頼できるテストが何より先に必要であることを実感することができました。

最後に改善の前後のソースの置き場所を貼っておきます。

改善前

改善後