凝集度のあるコード

コードレビューしてもらいました。

#encoding: cp932

ELDEST_YEAR = 1970
LASTEST_YEAR = 2030
ELDEST_MONTH = 1
LASTEST_MONTH = 12
ELDEST_DAY = 1
LASTEST_DAY = 31

week = [ "日曜日", "月曜日", "火曜日", "水曜日", "木曜日", "金曜日","土曜日"]

def get_int(eldest, lastest)
  s = gets()
  i = Integer(s)
  if i >= eldest && i <= lastest
    return i
  else
    puts("その数字は入力できません。もう一度入力してください。")
    i = get_int(eldest, lastest)
  end
end

puts("誕生年は？")
birth_year = get_int(ELDEST_YEAR, LASTEST_YEAR)
puts("誕生月は？")
birth_month = get_int(ELDEST_MONTH, LASTEST_MONTH)
puts("誕生日は？")
birth_day = get_int(ELDEST_DAY, LASTEST_DAY)
birthday= Time.local(birth_year, birth_month, birth_day)
birth_wday = birthday.wday()
puts ("誕生曜日は#{week[birth_wday]}です")
time_diff = Time.now() - birthday
puts("あなたはこれまで#{Integer(time_diff)}秒を過ごしてきました。")

こんなの。でもこれは良くないそうです。
なんで、良くないか。

  1. 1回しか使わない数字に定数を付ける必要はない
  2. 短命な変数ほど、使用するコードの近くに置く
  3. 名前の付け方が悪い

以上です。

  1. 1回しか使わない数字に定数を付ける必要はない

↓このあたり

ELDEST_YEAR = 1970
LATEST_YEAR = 2030
ELDEST_MONTH = 1
LATEST_MONTH = 12
ELDEST_DAY = 1
LATEST_DAY = 31

(省略)

puts("誕生年は？")
birth_year = get_int(ELDEST_YEAR, LATEST_YEAR)
puts("誕生月は？")
birth_month = get_int(ELDEST_MONTH, LATEST_MONTH)
puts("誕生日は？")
birth_day = get_int(ELDEST_DAY, LATEST_DAY)

この場合、数字自体に意味があるのでマジックナンバーにはならないから、定数にする必要がない。

(定数削除)


(省略)

puts("誕生年は？")
birth_year = get_int(1970, 2030)
puts("誕生月は？")
birth_month = get_int(1, 12)
puts("誕生日は？")
birth_day = get_int(1, 31)

あと、2/31にするとRubyは"2008-03-02 00:00:00 +0900"と計算しなおしてくれます。

  1. 短命な変数ほど、使用するコードの近くに置く
10　week = [ "日曜日", "月曜日", "火曜日", "水曜日", "木曜日", "金曜日","土曜日"]

(省略)

30　birth_wday = birthday.wday()
31　puts ("誕生曜日は#{week[birth_wday]}です")

weekの設定と実際に使用しているputs()の間には約20行もの距離がある。これは遠いでしょう。puts()でweekって何だ？と思ったら20行戻らなければいけない。プログラム内で使用回数が低いつまり、短命な変数であるほど使用する場所に近くすること。

29　week = [ "日曜日", "月曜日", "火曜日", "水曜日", "木曜日", "金曜日","土曜日"]
30　birth_wday = birthday.wday()
31　puts ("誕生曜日は#{week[birth_wday]}です")
  1. 名前の付け方が悪い
puts("誕生月は？")
birth_month = get_int(ELDEST_MONTH, LATEST_MONTH)

ELDEST_MONTHとLASTEST_MONTHはmin・maxにすべき。


アジャイルプラクティスによれば、「何らかの機能を一緒になって実現している要素同士は”凝集度が高い”という。クラスは狙いを絞り、コンポーネントは小さくすること」「問題は切り分けて考える。大きいプログラムほど有効」らしいです。
コード書くのって本当難しい。今日指摘されたことを常に意識して、普段から訓練すべきですね。