Ruby初心者必見！？「ビンゴカード作成問題」のリファクタリング風景をお見せします #codeiq

はじめに

先月、CodeIQにビンゴカード作成問題を出題しました。

CodeIQに「ビンゴカード作成問題」を出題しました。みなさんの挑戦をお待ちしてます！ - give IT a try

f:id:JunichiIto:20150209100505p:plain:w400

このビンゴカード作成問題、ありがたいことに50人もの方が解答を送ってくれました。
挑戦してくださったみなさん、どうもありがとうございました。

前回のエントリでは優秀作品ベスト3を発表しました。
今回のエントリはその続編です。
一部の解答（5本）について、僕が実際にいただいた解答を採点しつつ、リファクタリングする様子を動画に撮っておいたので、その様子をお見せしちゃいます。

f:id:JunichiIto:20150306085920g:plain

おさらい「ビンゴカード作成問題」とは？

ビンゴカード作成問題とはその名の通り、Rubyを使ってビンゴカードを出力する問題です。
Bingo.generate_cardというメソッドを呼ぶと以下のような文字列を出力する、というのが要求仕様です。

 B |  I |  N |  G |  O
13 | 22 | 32 | 48 | 61
 3 | 23 | 43 | 53 | 63
 4 | 19 |    | 60 | 65
12 | 16 | 44 | 50 | 75
 2 | 28 | 33 | 56 | 68

ビンゴカードには作成のルールがあります。

各列の値は以下の条件を満たすこと
- B：1～15のどれか
- I：16～30のどれか
- N：31～45のどれか
- G：46～60のどれか
- O：61～75のどれか

その他にも次のような条件があります。

毎回異なるカードを生成すること
どの数値も重複しないこと
各列はパイプ（|）で区切ること
数字や"BINGO"の文字は右寄せで出力すること
真ん中（FREEになる場所）はスペースを出力すること

予め用意されたテストを全部パスすればクリア！

解答を提出する際はこちらで予め用意したRSpecのテストを全てパスさせる必要があります。
RSpecのテストを含む解答テンプレートについてはこちらに置いてます。

ビンゴカード作成問題・解答テンプレート

動画の視聴設定について

今回公開した動画を視聴する場合は、画質をHDに、再生速度を倍速（2倍）にして設定するのがオススメです。

f:id:JunichiIto:20150306082948p:plain:w250

画質が荒いと字が潰れてコードが読みにくくなりますし、通常の速度だとちょっとダラダラしてしまうので、倍速で見るぐらいがさくっと最後まで視聴できてちょうどいいんじゃないかなと思います。

おまけ・RubyMineのリファクタリング機能も動画で見られる！

今回の採点作業でも普段愛用しているRubyMineを使っています。
特に強力なリファクタリング機能や、グラフィカルなテストの進捗表示なんかは普段テキストエディタしか使っていない人にとっては興味深いんじゃないかな～と思います。
RubyMineに興味がある方はそちらにも注目してみてください！

RubyMine: Ruby on Rails IDE by JetBrains
f:id:JunichiIto:20150307073135p:plain:w300

それではここから実際の解答例を紹介していきます。
今回紹介する解答例は全部で5本です。

解答例その1「他の言語の影響がちょっと強いかな？」

リファクタリング前

class Array
  def shuffle()
    shuffleBody(self.dup)
  end

  private
  def shuffleBody(inList)
    return [] if inList.size == 0

    retVal = []
    while inList.size > 0
      randID = rand(inList.size)
      retVal.push(inList.delete_at(randID))
    end
    return retVal
  end
end


class Bingo
  def self.setupInitialList(startNum, endNum)
    retList = []
    startNum.upto(endNum){|i| retList.push(i)}
    return retList
  end

  def self.generate_card
    numListSet = Hash::new
    numListSet["B"] = Bingo.setupInitialList(1, 15).shuffle().map{|i| sprintf("%2d",i)}
    numListSet["I"] = Bingo.setupInitialList(16, 30).shuffle().map{|i| sprintf("%2d",i)}
    numListSet["N"] = Bingo.setupInitialList(31, 45).shuffle().map{|i| sprintf("%2d",i)}
    numListSet["G"] = Bingo.setupInitialList(46, 60).shuffle().map{|i| sprintf("%2d",i)}
    numListSet["O"] = Bingo.setupInitialList(61, 75).shuffle().map{|i| sprintf("%2d",i)}

    numListSet["N"][2] =  "  "

    characterList = ["B", "I", "N", "G", "O"]
    buffer = ""
    buffer << characterList.map{|c| " #{c}"}.join(" | ") + "\n"

    0.upto(4){|i|
      lineBuffer = []
      characterList.each{|character|
        lineBuffer.push(numListSet[character][i])
      }
      buffer << lineBuffer.join(" | ") + "\n"
    }

    return buffer
  end

リファクタリング後

キャメルケースになっていた変数名やメソッド名をスネークケースに変換したり、コードをDRYにしたりしています。
また、Array#shuffleは車輪の再発明になっていたので削除させてもらいました。

class Bingo
  def self.generate_card
    range_array = [
        1..15,
        16..30,
        31..45,
        46..60,
        61..75
    ]
    col_values = range_array.map {|range| range.to_a.shuffle.map { |n| n.to_s.rjust(2) } }
    character_list = 'BINGO'.chars
    num_list_set = character_list.zip(col_values).to_h
    num_list_set["N"][2] =  "  "

    header = character_list.map {|c| c.rjust(2) }.join(" | ")

    body = 5.times.map {|i|
      character_list.map {|character| num_list_set[character][i] }.join(" | ")
    }

    [header, body].join("\n")
  end
end

動画で見るリファクタリング風景

Ruby - ビンゴカード作成問題・解答例その1 - YouTube

解答例その2「ブロックはネストさせすぎないように！」

リファクタリング前

class Bingo
  def self.generate_card
    title = %w(B I N G O)
    row = title.size
    range = 15

    title.map { |t| format('%2s', t) } * ' | ' + "\n" +
      (Array.new(row) do |i|
        Array.new(range) { |j| (i * range + j + 1) }.sample(row)
      end).transpose.map!.with_index do |l, i|
        l.map!.with_index do |r, j|
          (i != row / 2 || i != j) ? format('%2d', r) : '  '
        end * ' | '
      end * "\n"
  end
end

リファクタリング後

リファクタリング前のコードはぱっと見、短くまとまっているように見えますが、よく見ると後半はかなりブロックがネストしていて、何をやっているのかわかりづらいです。

そこでもう少しコードを分解して、コードの意図がわかりやすくなるようにリファクタリングしてみました。

class Bingo
  def self.generate_card
    title = 'BINGO'.chars
    col_size = title.size
    range = 15

    title_str = title.map { |t| t.rjust(2) } * ' | '

    numbers = col_size.times
                  .map { |i| range.times.map { |j| (i * range + j + 1) }.sample(col_size) }
                  .transpose

    body = numbers.map.with_index do |row, row_index|
             row.map.with_index { |col, col_index|
               center = row_index == col_size / 2 && row_index == col_index
               center ? '  ' : col.to_s.rjust(2)
             } * ' | '
           end

    [title_str, body].join("\n")
  end
end

動画で見るリファクタリング風景

Ruby - ビンゴカード作成問題・解答例その2 - YouTube

解答例その3「もっとDRYにできますよ！」

リファクタリング前

require 'set'

class Bingo
  ROW_MAX = 5

  def self.generate_card
    columns = []
    columns << unique_random_range(1..15, ROW_MAX)
    columns << unique_random_range(16..30, ROW_MAX)
    n_column = unique_random_range(31..45, ROW_MAX - 1)
    n_column.insert(2, " ")
    columns << n_column
    columns << unique_random_range(46..60, ROW_MAX)
    columns << unique_random_range(61..75, ROW_MAX)

    bingo = columns.transpose
    bingo.unshift(%w(B I N G O))

    formatted = bingo.map do |column|
      column.map {|s| sprintf "%2s", s }.join(" | ")
    end

    formatted.join("\n")
  end

private
  def self.unique_random_range(range, count)
    nums = Set.new
    while nums.size < count
      nums << rand(range)
    end
    nums.to_a
  end
end

リファクタリング後

こちらのコードもDRYになっていなかったので、どんどんDRYにしていきました。
それと unique_random_range メソッドのロジックはArray#sampleを使えばすぐに実現できそうだったので、メソッドごとなくしました。

class Bingo
  ROW_MAX = 5

  def self.generate_card
    range_array = [
        1..15,
        16..30,
        31..45,
        46..60,
        61..75
    ]
    columns = range_array.map {|range| range.to_a.sample(ROW_MAX) }
    columns[2][2] = ' '

    bingo = ['BINGO'.chars, *columns.transpose]

    bingo.map { |column|
      column.map {|s| s.to_s.rjust(2) }.join(" | ")
    }.join("\n")
  end
end

動画で見るリファクタリング風景

Ruby - ビンゴカード作成問題・解答例その3 - YouTube

解答例その4「Ruby初心者さん、いらっしゃい！」

リファクタリング前

class Bingo
  def self.generate_card
    s = " B |  I |  N |  G |  O\n"

    # 数値の生成
    num = []
    for i in 0...5 do
      num += generate_number(i*15+1)
    end

    # 出力
    for i in 0...5 do
      for j in 0...5 do
        if (num[j * 5 + i] < 10)
          s += " "
        end
        if (j * 5 + i == 12)
          s += "  "
        else
          s += num[j * 5 + i].to_s
        end
        if (j != 4)
          s += " | "
        end
      end
      s += "\n"
    end
    
    print(s)
    return s
  
  end
  
  # 指定された数値から+15までの範囲から、
  # 5つの数値を取得し、配列として返す
  def self.generate_number(start)
    num = []
    for i in start...start+15 do
      num << i
    end
    num.shuffle!
    return num[0, 5]
  
  end
end

リファクタリング後

解答者の方は今回初めてRubyを触ったそうです。
なので、forループが登場したりしています。

そこでロジックはほぼ変えずにRubyっぽい書き方や、Rubyが標準で提供しているAPIにどんどん置き換えていきました。

class Bingo
  def self.generate_card
    # 数値の生成
    numbers = 5.times.flat_map do |i|
      start = i * 15 + 1
      [*start..(start + 14)].sample(5)
    end

    # 出力
    body = 5.times.map do |row|
      5.times.map { |col|
        center = col * 5 + row == 12
        center ? "  " : numbers[col * 5 + row].to_s.rjust(2)
      }.join(' | ')
    end
    title = " B |  I |  N |  G |  O"
    [title, body].join("\n")
  end
end

動画で見るリファクタリング風景

Ruby - ビンゴカード作成問題・解答例その4 - YouTube

解答例その5「マス目の上限が自由に変えられます」

リファクタリング前

# 
# 出題は、5 x 5 の升目固定でしたが
# タイトルの文字数と使える数の上限を指定できる
# ようにクラスメソッド generate_card_flexible 
# を定義
# generate_card からは、generate_card_flexibleを
# コールするようにしています
# 
class Bingo
  def self.generate_card
    # 以下の行は削除して、自分でロジックを実装してください。
    generate_card_flexible('BINGO', 75)
    # generate_card_flexible('LARGE BINGO', 400)
    # generate_card_flexible('LARGER BINGO', 450)
  end

  #
  # generate_card_flexible(header, n) -> String
  #   ビンゴカードを生成するメソッド
  #   第1引数にビンゴカードのヘッダを指定。
  #   第2引数にビンゴカードで使用できる数字の上限
  #   を指定する。
  #   第1引数で指定されたヘッダの文字数に合わせた
  #   ビンゴカードを生成する。
  #   ヘッダの文字数が偶数のときはヘッダの先頭に
  #   空白が追加される。
  #   ヘッダの文字に指定できるのは半角のみ。
  #   全角文字には対応していない。
  #
  def self.generate_card_flexible(header, n)
    
    # ヘッダーの文字列からビンゴの1辺のサイズを求める。
    len = header.length

    # 1辺のサイズが偶数だと真ん中がなくなるので
    # 1辺のサイズを1増やす
    # ヘッダーは空白を1文字先頭に付加する
    if len % 2 == 0
      len += 1
      header = ' ' + header
    end

    # 数字の個数がビンゴの升目より少ない
    # ときはエラーにする。
    if n < len * len
      raise "can't generate card!!! #{n} must be larger than #{len*len}."
    end

    # 1 から n までの数字をlen等分する。
    ary = (1..n).to_a.each_slice(n/len).to_a
    
    # 数字が綺麗に(n/len)個ずつ分割できなかったときの微調整。
    # 最後の2つをまとめることにする。
    # (今回は考慮しなくてもよいけど。)
    ary[-2] += ary[-1] if ary.size > len 

    # ランダムに数字を5個ずつ抽出して
    # ヘッダーの1文字を先頭につけて
    # 文字幅を微調整する。
    # ヘッダーの文字は半角のみであることを想定しているけど、
    # 今回はそれでよしとする。
    cols = (0..len-1).map {|i|
      ary[i].sample(len).unshift(header[i]).map{|i|
        i.to_s.rjust(n.to_s.length)
      }
    }

    # 真ん中をブランクにする。
    cols[len/2][len/2 + 1] = ' '.rjust(n.to_s.length)

    # 行と列を入れ変えて" | "で区切って
    # 1つの文字列にして完成
    cols.transpose.map {|ary|
      ary.join(" | ") 
    }.join("\n")
  end
end

リファクタリング後（省略）

この方は独自に問題を拡張して、マス目の上限を自由に変更できるロジックを書いてくれました。
なのでちょっとコードが長くなっています。

ただ、コードは長いものの、コード自体はすごくきれいに書かれてあるので、ほとんど手直しする部分がありませんでした。
というわけで、リファクタリング後のコードは省略させてもらいます。

動画で見るリファクタリング風景

Ruby - ビンゴカード作成問題・解答例その5 - YouTube

今回紹介する解答例は以上です。
挑戦してくださったみなさん、どうもありがとうございました！

ここからあとは総評を兼ねて、きれいなコードを書くためのポイントを説明していきます。

シンプルでスマートなコードを書くポイント

50人もの解答を見ていると、何となく「こんなふうに書けばいいのになー」と感じるポイントが似通ってきます。
それは一体何なのか？具体的なポイントを見ていきましょう。

Rubyの標準APIを活用する

RubyのStringやArray、Enumerableには便利なメソッドがたくさんあります。
思いついたロジックをコードとして表現する前に、まず標準APIを使ってラクができないか調べてみましょう。

たとえば今回の問題であれば、以下のようなメソッドの出番が多いと思います。

Enumerable#map
Array#sample
Array#join
Array#transpose
String#rjust

コードを書くときはAPIドキュメントをよく読んで、車輪の再発明をしていないか調べる癖を付けましょう。

Ruby 2.2.0 リファレンスマニュアル > ライブラリ一覧 > 組み込みライブラリ

また、僕が以前Qiitaに書いたこの記事も参考になると思います。

[初心者向け] RubyやRailsでリファクタリングに使えそうなイディオムとか便利メソッドとか

徹底的にDRYにする

みなさんからもらった解答を見ていると、今回はこんなコードを書いている人がたくさんいました。

bingo = [
  (1..15).to_a,
  (16..30).to_a,
  (31..45).to_a,
  (46..60).to_a,
  (61..75).to_a,
]

きれいと言えばきれいなんですが、to_aを何度も書いてしまっています。
これはmapを使えばDRYにできます。

bingo = [
  1..15,
  16..30,
  31..45,
  46..60,
  61..75
].map(&:to_a)

さらに言えば、これらの数字は15ずつ増える、という規則性を持っているので、ここもDRYにできます。

bingo = (1..75).each_slice(15).to_a

each_sliceは引数で与えた数の分だけ配列等のコレクションを分割するメソッドです。
ここでは1から75までの数字を15個ずつ分割して、配列の配列を作っています。

他にも区切り文字の縦棒（|）や改行文字（\n）もうまくやればコード中に1回しか出てこないようにすることができます。
自分の書いたコードに縦棒や改行文字が2回以上出てきている場合はまだDRYにできる余地があるということです。
さらに自分のロジックに磨きをかけましょう！

最初から完璧なコードは目指さなくてよい

上に書いたようなアドバイスを読むと「最初からそんなに上手に書けるわけないよ！」と思う人がいるかもしれません。
でも大丈夫です。必ずしも最初から完璧を目指さなくて構いません。
最初はぱっと思いついたコードを書き、あとからどんどんリファクタリングする、というアプローチも全然アリです。

実は僕も最初のコミットではこんなコードを書いていました。

class Bingo
  def self.generate_card
    b = ['B', *(1..15).to_a.shuffle.take(5)]
    i = ['I', *(16..30).to_a.shuffle.take(5)]
    n = ['N', *(31..45).to_a.shuffle.take(5)]
    g = ['G', *(46..60).to_a.shuffle.take(5)]
    o = ['O', *(61..75).to_a.shuffle.take(5)]
    cols = [b, i, n, g, o].map{|array| array.map{|s| s.to_s.rjust(2)}}
    cols[2][3] = '  '
    rows = cols.transpose
    rows.map{|array| array.join(' | ')}.join("\n")
  end
end

それが最終的にはこんなコードになりました。

class Bingo
  FORMAT = Array.new(5, '%2s').join(' | ')

  def self.generate_card
    ['BINGO'.chars, *numbers].map { |row| FORMAT % row }.join("\n")
  end

  def self.numbers
    (1..75).each_slice(15)
      .map { |sequence| sequence.sample(5) }
      .tap { |table| table[2][2] = '' }
      .transpose
  end
end