【アルゴリズム】【Ruby】アルゴリズムのリファクタリング
こんなコードでイケテるかなー?って朝にコードを書いていたら、師匠からメールが来た。
コードは大体いいと思う。 リファクタリングするなら 後置if を使う kaisu はiで代用する あと、エラーメッセージが変?
ふむ。順に見てみよう。
■元のコード(抜粋)
a.each_with_index do |int, i| if int<0 or int>9999 break else if min >int min = int min_doko = i end if max < int max = int max_doko = i end kaisu += 1 end end if kaisu == 0 p "だめな数しか入ってない" exit end
改善:エラーメッセージ
エラーメッセージが変?
確かに、今は kaisu == 0のときにエラーメッセージが出て処理がとまる作り。
”いきなりだめな数”のときエラーが発生しているわけだから、
MSGもそう直そう。
■改善後
if kaisu == 0 p "ikinari dame na kazu" exit end
改善:後置if を使う
後置if は、使えないのでは。
書いているif文は、
- else文があるもの
- ifブロック内の処理が2文以上
だから。
改善:kaisu はiで代用する
うーん。確かに、kaisuは結局、
「初回が正しくない数(負数・1万以上)だったらエラーを出す」
にしか使っていないので、
ぐるぐる回っている中で、このエラー判定をすればよい。
■改善後
a.each_with_index do |int, i| if int<0 or int>9999 if i == 0 p "ikinari dame na kazu" ←ここに移動 exit else break end else if min >int min = int min_doko = i end if max < int max = int max_doko = i end end end
こういうことを言いたかったのかな。
でも処理は少し見えにくくなった気もする。
■修正後全量コード
a =(0...10000).to_a a =( a.sample(4) << -1).sample(5) p a min = 10000 max = -1 min_doko = 0 max_doko = 0 kaisu = 0 a.each_with_index do |int, i| if int<0 or int>9999 if i == 0 p "ikinari dame na kazu" exit else break end else if min >int min = int min_doko = i end if max < int max = int max_doko = i end end end p "min" + min.to_s p "max" + max.to_s p "min_doko" + min_doko.to_s p "max_doko" + max_doko.to_s
よくなったかも?