閏年判定のプログラムを採点してください

Question

Rubyで閏年判定のアルゴリズムを書きました。

命名やクラス化、アルゴリズムの観点から良いコードか悪いコードか採点して欲しいです。

class Leap
  def checkYear?(n)
    if n%400 == 0
      return true
    end

    if n%4 == 0 && n%100 == 0
      return false
    end

    if n%4 == 0
      return true
    end
 
    return false
  end
end

leap =  Leap.new
p "閏年" if leap.checkYear?(gets.to_i)

また、自分だったらこういう風に書くというのを教えていただきたいです。できればアドバイス等も頂きたいです。

Answer 1

私が書く場合を示します。
(でも、そもそも うるう年判定なら、ruby にすでにメソッドがあるので、自分では書くことは無いとおもいますが)

leap.rb

require 'date'

class Leap
  def self.check?(year)
    (year % 400 == 0) || (year % 100 != 0 && year % 4 == 0)
  end
end

TESTS = {
  1998 => false,
  1999 => false,
  2000 => true,
  2001 => false,
  2004 => true,
  2008 => true,
  2100 => false
}.freeze

TESTS.each do |year, result|
  actual = Leap.check?(year)
  puts "ERROR: #{year} -> #{actual}" if actual != result
end

  # ruby の Date クラスのうるう年判定メソッドと比較する
(1900..2400).each do |year|
  result = Leap.check?(year)
  result2 = Date.leap?(year)
  puts "ERROR: #{year} Date.leap?() != Leap.check?()" if result != result2
end

変更したのは次の点です。

1. メソッド名を変更しました。

--> まずは checkYear? でなく check_year? に変更しましたが、さらに check? にしました。
(本当は、クラス名も変えて、それに合わせてメソッドもさらに別のものに変えたいところですが ...)
2. インスタンスに依存した処理ではないので、クラス・メソッドにしました。
3. 判定を 条件文１つにまとめました。
4. キー入力して試すのでなく、プログラム内部で主な年について試すようにしました。
（ここではさらに、ruby の Data.leap? との動作チェックもしてみました)

参考情報：

• Rubyで『ある年のある月の日数を求めるメソッド』を作成したのですが... https://teratail.com/questions/20280
• Rubyで閏年問題プログラムを作ってみようと思い書いてみました https://teratail.com/questions/3646
• http://codereview.stackexchange.com/questions/23902/

Comments

[raccy]

Date.leap?なんてものがあったとは・・・

Answer 2

アルゴリズムの観点からと言うことなので、その観点だけでコメントします。
約3/4のケースは閏年で無いので、そのケースを最初に判定すべき。
まあ、このケースだと微々たる差ですが、一般論としては、少数ケースの中の場合分けにコストが掛かる場合は効いてきます。

def checkYear?(n)
    if n%4 != 0
      return false
    end

    if n%400 == 0
      return true
    end

    if n%100 == 0
      return false
    end

    return true
end

Comments

[majiponi]

その考え方に立つなら、4で割り切れる場合でも、先に100で割り切れないから閏年に確定、とするほうがもっと速くなりますよね?

[otn]

ああ、確かにそうですね。指摘ありがとうございます。

Answer 3

閏年の条件を忘れてしまった！
と言うときは、Dateを使うという方法もあります。

require 'date'
def check_leap_year(year)
  Date.valid_date?(year, 2, 29, Date::GREGORIAN)
end

アルゴリズムを考える勉強にはなりませんが、便利なライブラリは積極的に使うと楽ができます。

Answer 4

Q1. クラスに入れてある理由を説明してください。
(単純な関数ではダメですか?)

Q2. 400で割り切れる数は、100でも4でも割り切れますよね?
(無駄な判定がある)

Q3. 2001年を引数にした場合の結果は何ですか?
(漏れがある)

def isLeapYear(year)
    if year%400 == 0
        return true
    end

    if year%100 == 0
        return false
    end

    if year%4 == 0
        return true
    end

    return false
end

Comments

[退会済みユーザー]

ご回答ありがとうございます。

Q1につて
単純な関数でもいいのですが、今後パッケージ化などした際に扱いやすいと思ったのでクラス化しました。

Q2について
コード修正するので確認お願いします。

Q3について
何も帰ってきませんが、問題があるのでしょうか？

よろしくお願いします。

[majiponi]

Q1.補足
クラスにまとめるということには、オブジェクトと何かしらの関係がある、ということです。例えば時刻、日付を扱うオブジェクトに関するクラスでしたら、閏年の判定は意味があります。ですが、ただ単に閏年か判定するだけのオブジェクトを作るクラスって、必要ですか?

Q2.補足
あくまで例示です。100で割り切れたら4で割り切れることは省略して書いてます。

Q3.補足
例えば、2000年でしたらtrueが返ってきて、1900年でしたらfalseが返ってきますよね。2016年ならtrueが返ってきて、2015年なら…あれ、何も返ってきませんね? そういうことです。falseという値を返す意味がある、ということです。呼び出し側ではその年が閏年(true)か?という判定法以外に、平年(false)か?という判定法も考えられますから。

[退会済みユーザー]

返信ありがとうございます。

Q1について
なるほど。つまり現状だと閏年を返す関数のみしかないので、クラス化する必要はないということですか？今後このクラスに様々な関数が作られ、閏年判定に関係するならばクラス化は有効だということですか？

Q2について
コード修正して思ったのですが、閏年はそもそも4と100で割り切れ、且つ400でも割り切れる時を示すと思います。確かに400で割切れるなら4,100の計算は無駄になります。
しかし、コードを他者が見た時に、このif文ではどういう処理なのか、どういう意図のコードなのか分からない気がするのですが。いかがでしょう？

Q3について
おっしゃる通りでした。すべての条件式に該当しない場合、falseを返すようにしました。

[majiponi]

Q2補足の補足
なら、コメントできちんと書けばいいのです。(私は関数名で何をやっているか分かるので、コメントで補足する必要すらないと思いますが)
そもそも、例外中の例外から判定しているからそうなるのであって、他の方が指摘するように、原則(通常の平年)、例外の原則(4で割り切れて100で割り切れない)、例外の例外(100で割り切れて400で割り切れない)、例外の例外の例外、の順に書けば特に問題ありませんよね?

[退会済みユーザー]

返信ありがとうございます。
確かに確率の大きい事象から判定するのが良さそうですね。
今後気をつけます！