とある Ruby ソースのリファクタリングについて
前回の記事の続きです。
とあるプルリクエストで採用されたコードは、無駄がいっぱいあったのと
メンテナンス性が悪そうだったので、リファクタリングしてみました。
まず、以下の問題がありました。
10000 回ほど、連続して実行させると結果が少し偏る
$ ./roulette.rb M2 ふじい: 2937 すがわら: 2023 おくつ: 2662 なかじま: 2378 $ ./roulette.rb M2 ふじい: 3026 なかじま: 2366 すがわら: 2035 おくつ: 2573 $ ./roulette.rb M2 ふじい: 3089 すがわら: 2074 なかじま: 2183 おくつ: 2654
上記は、rand(10) を rand(100)にすることで結果が(少し)均等になりました。
$ ./roulette.rb M2 ふじい: 2521 すがわら: 2379 なかじま: 2526 おくつ: 2574 $ ./roulette.rb M2 おくつ: 2527 ふじい: 2545 なかじま: 2396 すがわら: 2532 $ ./roulette.rb M2 すがわら: 2398 おくつ: 2474 なかじま: 2470 ふじい: 2658
次に、使用者の目線で考えると Roulette クラスは複数のインスタンスを
生成したりすることはなさそうなので、new する必要もないだろうということで
start/speak は、クラスメソッド化しました。
おかげさまで、Roulette.new(arg).start ではなくて
Roulette.start(arg) という風に自然な感じの使い方が出来そうです。
最後に、やたら出てくるコマンドのパスがメンテナンス性を下げていたため
定数に変更し、メンテナンス性を高めました。
以下は、再度プルリクエスト中のソースコードです。
#!/usr/bin/env ruby # -*- coding: utf-8 -*- SAY = "/usr/bin/say" PLAYER = "/usr/bin/afplay" BGM = "./gaki.mp3" group = { "M2" => %w(おくつ すがわら なかじま ふじい), "M1" => %w(たけみち こまつ ふるだて), "B4" => %w(おくとも かとう いのうえ あべ なかむら ひらが まいた かどわき ほし やぎぬま) } class Roulette def self.start members @result = Hash.new @members = members @members.each do | member | @result[member] = rand(100) + 1 end @result = @result.sort{|a, b| b[1] <=> a[1]} if File.exist?("#{SAY}") speak end return @result end private def self.speak result = @result.dup result.each_with_index do | elem, index | 3.times { print "."; sleep(0.5) } puts message = "#{elem.join(", ")}点。" `#{SAY} #{message}` if index == result.length - 1 threads = [] puts message = "#{elem[0]}" + "ーーー。アウトーーー。" threads.push(Thread.new{`#{PLAYER} #{BGM}`}) sleep(4) threads.push(Thread.new{`#{SAY} #{message}`}) threads.each{|thread| thread.join} end end end end attr = ARGV[0] if group.include?(attr) Roulette.start(group[attr]) else Roulette.start($*) end
とりあえず、現状これがせいいっぱいなんですが
もっとリファクタリングできるよ!ってな人はコメントお願いします。