Faker::Date.birthday has some issues

[tagliala]

Hi,

thanks for this gem!

I have noticed something wrong in the birthday faker.

1. from and to should be inverted

      def birthday(min_age = 18, max_age = 65)
        t = ::Date.today
        top_bound, bottom_bound = prepare_bounds(t, min_age, max_age)
        years = handled_leap_years(top_bound, bottom_bound)

        from =  ::Date.new(years[0], t.month, t.day)
        to   =  ::Date.new(years[1], t.month, t.day)
        debugger

        between(from, to).to_date
      end

$ rake test
(byebug) min_age
18
(byebug) max_age
65
(byebug) from
#<Date: 1996-04-06 ((2450180j,0s,0n),+0s,2299161j)>
(byebug) to
#<Date: 1952-04-06 ((2434109j,0s,0n),+0s,2299161j)>

2. Tests are a little bit confusing

      t = Date.today
      date_min = Date.new(t.year - min, t.month, t.day)
      date_max = Date.new(t.year - max, t.month, t.day)
      birthday = @tester.birthday(min, max)
      assert birthday >= date_max, "Expect > \"#{date_max}\", but got #{birthday}"
      assert birthday <= date_min, "Expect > \"#{date_max}\", but got #{birthday}"

Given that the failure message is wrong (> #{date_max} in both cases), variables are confusing

date_min represent the birthdate of the younger man, so the maximum birthdate. date_max represent the birthdate of the older man, so the minimum birthdate.

Have birthday <= date_min is the right test, but it is confusing. I would rename those variables to something like

      birthdate_min = Date.new(t.year - max, t.month, t.day)
      birthdate_max = Date.new(t.year - min, t.month, t.day)
      birthday = @tester.birthday(min, max)
      assert birthday >= birthdate_min, "Expect >= \"#{birthdate_min}\", but got #{birthday}"
      assert birthday <= birthdate_max, "Expect <= \"#{birthdate_max}\", but got #{birthday}"

3. The method is broken for newborns (and generally when max_age - min_age < 5)

$ rails c
Running via Spring preloader in process 96897
Loading development environment (Rails 5.1.0.rc1)
[1] pry(main)> Faker::Date.birthday(0, 3)
=> Wed, 06 Apr 2016
[2] pry(main)> Faker::Date.birthday(0, 3)
=> Wed, 06 Apr 2016
[3] pry(main)> Faker::Date.birthday(0, 3)
=> Wed, 06 Apr 2016
[4] pry(main)> Faker::Date.birthday(0, 3)
=> Wed, 06 Apr 2016
[11] pry(main)> Faker::Date.birthday(3, 5)
=> Fri, 06 Apr 2012
[12] pry(main)> Faker::Date.birthday(3, 5)
=> Fri, 06 Apr 2012
[13] pry(main)> Faker::Date.birthday(3, 5)
=> Fri, 06 Apr 2012

The problem is not only due to the inversion of form and to. The call to handled_leap_years(top_bound, bottom_bound) returns [2016, 2016] when min_age = 0 and max_age = 1

I will attach a PR to fix issues 1 and 2, I need help for the 3rd issue

Sorry for my poor English

[andrewmbyrd]

I'm looking into the 3rd issue now. Will submit a PR soon

[andrewmbyrd]

I believe I've solved it.

2.3.2 :001 > require 'faker'
 => true 
2.3.2 :002 > Faker::Date.birthday(0,3)
 => #<Date: 2016-05-11 ((2457520j,0s,0n),+0s,2299161j)> 
2.3.2 :003 > Faker::Date.birthday(0,3)
 => #<Date: 2016-12-07 ((2457730j,0s,0n),+0s,2299161j)> 
2.3.2 :004 > Faker::Date.birthday(0,3)
 => #<Date: 2015-01-29 ((2457052j,0s,0n),+0s,2299161j)> 
2.3.2 :005 > Faker::Date.birthday(0,3)
 => #<Date: 2014-07-21 ((2456860j,0s,0n),+0s,2299161j)> 
2.3.2 :006 > Faker::Date.birthday(3,5)
 => #<Date: 2012-06-25 ((2456104j,0s,0n),+0s,2299161j)> 
2.3.2 :007 > Faker::Date.birthday(3,5)
 => #<Date: 2012-12-03 ((2456265j,0s,0n),+0s,2299161j)> 
2.3.2 :008 > Faker::Date.birthday(3,5)
 => #<Date: 2012-06-03 ((2456082j,0s,0n),+0s,2299161j)> 
2.3.2 :009 > Faker::Date.birthday(3,5)
 => #<Date: 2013-08-14 ((2456519j,0s,0n),+0s,2299161j)> 
2.3.2 :010 > Faker::Date.birthday(0,0)
 => #<Date: 2017-05-16 ((2457890j,0s,0n),+0s,2299161j)> 
2.3.2 :011 > Faker::Date.birthday(0,0)
 => #<Date: 2017-05-16 ((2457890j,0s,0n),+0s,2299161j)> 
2.3.2 :012 > 

The issue with the previous implementation was in the handled_leap_years function. Because the current year is 2017, anytime max_age - min_age < 5 meant that the min year would be at least 2013. So in all of those cases, we can use this function call:

def handled_leap_years(2017, 2013)
        if (top_bound % 4) != 0 || (bottom_bound % 4) != 0
          [
            customized_bound(top_bound),
            customized_bound(bottom_bound, true)
          ]
        else
          [top_bound, bottom_bound]
        end
  end

Because 2017 and 2013 are not evenly divisible by 4, a call to customized_bound is made.

def customized_bound(bound, increase = false)
        if (bound % 4) != 0
          bound += 1 if increase
          bound -= 1 unless increase
          customized_bound(bound, increase)
        else
          bound
        end
end

The result of this will be that 2013 is increased until it gets to 2016. 2017 is decreased by one to get to 2016. (2016 % 4 = 0 so that's when the loop stops). So then when the "random" date is generated, it's range is from (today's date in year 2016) to (today's date in year 2016). Which can only give us today's date in 2016.

This same logic holds true for any leap year when max_age - min_age < 5

I've implemented the solution in my pull request

[tagliala]

Hi 👋

I've updated my PR #874 and now it contains a full fix for all the three issues

I've used @andrewmbyrd's approach and added a couple of tests to make sure this doesn't happen again

[anthony-bernardo]

Still buggy (version 1.8.7)

irb(main):001:0> require 'faker'
=> true
irb(main):002:0> Gem.loaded_specs["faker"].version
=> #<Gem::Version "1.8.7">
**irb(main):003:0> Faker::Date.birthday(3, 5)
=> Sat, 03 Oct 2015**
irb(main):004:0> Faker::Date.birthday(3, 5)
=> Wed, 12 Nov 2014
irb(main):005:0> Faker::Date.birthday(3, 5)
=> Fri, 09 May 2014

This is wrong :

irb(main):003:0> Faker::Date.birthday(3, 5)
=> Sat, 03 Oct 2015

The age is 2. http://www.calculator.net/age-calculator.html?today=10%2F03%2F2015&ageat=02%2F12%2F2018&x=37&y=24

[tagliala]

This has not been released yet