mysterioustrousers / MTDates

A thread safe date calculation library with all the date functions you'll ever need.
BSD 2-Clause "Simplified" License
343 stars 68 forks source link

Weekday calculation doing something wrong? #36

Closed Alex9779 closed 10 years ago

Alex9779 commented 10 years ago

Hi, I have a problem when doing calculation with weeks and weekdays when you you library and I am not able to track down why. I did a test:

NSDate *now = [NSDate date];
expectedDate = [NSDate mt_dateFromYear:[now mt_year]
                                  week:[now mt_weekOfYear]
                               weekday:[now mt_weekdayOfWeek]
                                  hour:[now mt_hourOfDay]
                                minute:[now mt_minuteOfHour]];

From what I feel expectedDate should be the same as today but it is yesterday. Maybe I have to add that I am in Europe and when I debug I can see the error: [now mt_weekdayOfWeek] returns 3 for wednesday as if 1 is monday but 3 is tuesday when sunday is 1. So the function returns one day less and recreating it with the value seems to use sunday as 1 so I get tuesday in the result, not wednesday. I dunno how to fix this or why it is this way or what I should have to set to get the correct result.

Regards, Alexander

atomkirk commented 10 years ago

Can I see a concrete example of how you're using these methods in a real app?

Alex9779 commented 10 years ago

Ahh well I tried to simplify my current method because it is not very nice so I included your lib. Unfortunately I didn't save my tries. What I am trying to do is I have a date in the past and I want to calculate a new date by adding a number of weeks to the date now but I want to keep the weekday, hour and minute of the date in the past. E. g. I have 2014-02-04 15:50:00, today is 2014-02-05 22:00:00 and I wanna add 2 weeks. My expected result is 2014-02-18 15:50:00 but with the current implementation I get 2014-02-17 15:50:00. What I tried is not much different from what I wrote in my first post but I add 2 to the week component and take the year from the now date and the weekday of the original date, but as I told the function that gets the weeks day gets it with 1=monday, in this case the result is 2, and the function that creates the new day uses 1=sunday, so the constructed date has monday as weekday...

Alex9779 commented 10 years ago

Ok here some code:

self.dueDate = [NSDate mt_dateFromYear:[now mt_year]
                                  week:[now mt_weekOfYear] + self.intervalAmount
                               weekday:[self.dueDate mt_weekdayOfWeek]
                                  hour:[self.dueDate mt_hourOfDay]
                                minute:[self.dueDate mt_minuteOfHour]];

dueDate is some date in the past, now is [NSDate date].

And here the code of my test:

expectedDate = [NSDate mt_dateFromYear:[now mt_year]
                                  week:[now mt_weekOfYear] + 2
                               weekday:[[task dueDate] mt_weekdayOfWeek]
                                  hour:[[task dueDate] mt_hourOfDay]
                                minute:[[task dueDate] mt_minuteOfHour]];

now is again [NSDate date], [task dueDate] is some date in the past...

Alex9779 commented 10 years ago

Some time passed and I tried myself some things. I wrote a test in the test suite:

NSDate *now = [[NSDate date] mt_startOfCurrentMinute];
NSDate *expectedDate = [NSDate mt_dateFromYear:[now mt_year]
                                          week:[now mt_weekOfYear]
                                       weekday:[now mt_weekdayOfWeek]
                                          hour:[now mt_hourOfDay]
                                        minute:[now mt_minuteOfHour]];

STAssertEqualObjects(now, expectedDate, nil);

and found that with no changes the test passes. But your tests set the default to local "en_US" and so the resulting calendar object has 1 as first weekday. I am located in Germany with the local "de_DE" and our first weekday is 2, monday. So when I use:

[NSDate mt_setFirstDayOfWeek:2];

in the test the test fails. I tracked it down and it seems to me that there is some calculation error in the calendar methods "- (NSUInteger)ordinalityOfUnit:(NSCalendarUnit)smaller inUnit:(NSCalendarUnit)larger forDate:(NSDate )date" and "- (NSDate )dateFromComponents:(NSDateComponents *)comps". The first one is used to get the weekday in "- (NSUInteger)mt_weekdayOfWeek" the second one when creating the new date from the components. The first one returns a number of the weekday always based on 1=sunday, 2=monday and so on. But the second one uses the set first weekday as 1. So when you do "[NSDate mt_setFirstDayOfWeek:2];" you have to construct with 1=monday, 2=tuesday and so on. I tried with other first weekdays and the test always fails if I do not first day of week to 1.

So I think there should be a correction in "- (NSUInteger)mt_weekdayOfWeek" as follows:

weekdayOfWeek += [[NSDate mt_calendar] firstWeekday] - 1;

This will correct the return value by the difference of the set first weekday and the test will always succeed...

Regards, Alexander

Alex9779 commented 10 years ago

After the fix from the previous post I tried to run all other test and realized a failure in "- (NSDate )mt_startOfCurrentWeek" Because we get now alway the "normalized" weekday back from "- (NSUInteger)mt_weekdayOfWeek" there has to be another fix in "- (NSDate )mt_startOfCurrentWeek":

NSDate *date = [self mt_dateDaysAfter:-(weekday - [[NSDate mt_calendar] firstWeekday])];

After that all tests succeed...

Alex9779 commented 10 years ago

I am preparing a pull request...

atomkirk commented 10 years ago

Nice catch! Yeah if you make changes to the code, write new tests that pass without modifying the other tests, ill merge it right in and push a new version of the cocoapod— Sent from Mailbox for iPhone

On Sat, Feb 15, 2014 at 9:48 AM, Alexander Leisentritt notifications@github.com wrote:

I am preparing a pull request...

Reply to this email directly or view it on GitHub: https://github.com/mysterioustrousers/MTDates/issues/36#issuecomment-35160765