せかいや

いまいるここを、おもしろく http://sekai-in-the-box.appspot.com/

【アルゴリズム】【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


よくなったかも?