#14704 closed enhancement (fixed)

New function to get difference of two wxDateTime objects as a wxDateSpan

Reported by: jonasr Owned by:
Priority: normal Milestone:
Component: base Version:
Keywords: wxDateTime Cc:
Blocked By: Blocking:
Patch: yes

Description

Hi

A new function to wxDateTime similar to

wxTimeSpan Subtract(const wxDateTime &dt)

which returns a wxDateSpan with all fields filled:

wxDateSpan SubtractDate(const wxDateTime &dt)

Attachments (5)

subtractdate.patch download (2.4 KB) - added by jonasr 22 months ago.
diffasdatespan.patch download (3.7 KB) - added by jonasr 22 months ago.
diffasdatespan2.patch download (3.8 KB) - added by jonasr 22 months ago.
gettotalmonths.patch download (1.2 KB) - added by jonasr 22 months ago.
diffasdatespan_fix.patch download (3.3 KB) - added by jonasr 22 months ago.

Download all attachments as: .zip

Change History (16)

Changed 22 months ago by jonasr

comment:1 Changed 22 months ago by vadz

  • Status changed from new to confirmed
  • Summary changed from New function to wxDateTime to get wxDateSpan to New function to get difference of two wxDateTime objects as a wxDateSpan

Thanks for your patch, I agree that this could be useful.

But I'm not sure if SubtractDate() is really a good name for it, I really would be hard pressed to explain how is it different from just Subtract(). I can propose SubtractLogical() but this is not ideal neither so what about a longer but perfectly clear DiffAsDateSpan()? It is inconsistent with Subtract(), of course, but OTOH it's really a different operation so I think clarity is more important than consistency here. But any other suggestions for the name would be welcome.

Also, and this is really important, could you please add a few tests for the new function to tests/datetime/datetimetest.cpp? If you have trouble with this, please see the instructions.

TIA!

comment:2 Changed 22 months ago by jonasr

I'll think about the name. I wanted it to be as close to Subtract as possible, but perhaps i'ts too close.

As for tests, I didn't find any tests for Subtract/Add in datetimetest.cpp, and the size of it, and its embedded python scripts, scared me.
But now that i revisit it, I realize that operator- is an alias for subtract, so now i feel dumb. I'll try to cook up some tests.

comment:3 Changed 22 months ago by jonasr

Writing the test I noted that arithmetics with the resulting wxDateSpan gives wrong results.

Consider this:

wxDateTime dt1(11, wxDateTime::Jan, 1996);
wxDateTime dt2(5, wxDateTime::Jun, 1998);
wxDateSpan diff = dt2.DiffAsDateSpan(dt1);  
// now diff has years=1, months=28, weeks=125, days=0
wxDateTime dt3 = dt1 + diff;
// now dt3 = 2001-10-02, so dt3 != dt2

This is because DiffAsDateSpan fills all fields that it can, so calls to GetYears()/GetMonths()/GetDays() etc. returns correctly for the diff.
But wxDateSpan::Add(const wxDateSpan&) uses all fields for adding, so it effectivly adds the span four times.
This maybe is somewhat unexpected, so i make a not of it in the documentation.

This means that writing a test that checks arithmetics is not possible, so the only thing I can think of is testing that the calculation of year/month/week/day is correct, hope that's ok.

Changed 22 months ago by jonasr

comment:4 Changed 22 months ago by vadz

Err, I think the implementation is wrong then (hence the usefulness of writing tests!). I'd expect the operation above to give "2 years, 4 months, 25 days", i.e. the number of months should never be greater than 12 in wxDateSpan. And if it worked like this, then addition would work, as expected.

comment:5 Changed 22 months ago by jonasr

For my purpose, I needed to get a span that returns the actual diff between two dates for each part of GetYears()/GetMonths()/GetWeeks()/GetDays(), so I wanted GetMonths() to return 28 in example above. (btw, i see it's a typo in the example, year in diff is ofcourse 2, not 1)

But I guess that this is a misunderstanding/misuse of wxDateSpan, and I agree that arithmetics should work as expected.
Now this function is of no use to me, but if someone else finds it useful I made a new patch.

Note that for the tests to pass, and to keep with how I understand wxDateSpan, weeks are set, so it's not exactly like your example, but rather "2 years, 4 months, 3 weeks, 4 days"

Changed 22 months ago by jonasr

comment:6 Changed 22 months ago by vadz

I think you may want to add wxDateSpan::GetTotalMonths() method, just as already have GetTotalDays(). Then you should be able to get the values that you need. Of course, it's not exactly difficult to write 12*GetYears()+GetMonths() directly neither but I wouldn't be against adding such method if people think it can be useful.

In the meanwhile I'll apply this patch, thanks!

comment:7 Changed 22 months ago by VZ

  • Resolution set to fixed
  • Status changed from confirmed to closed

(In [72600]) Add wxDateTime::DiffAsDateSpan().

This method returns the difference between the dates as wxDateSpan, unlike the
existing Subtract() and overloaded operator-() that return wxTimeSpan.

Closes #14704.

comment:8 Changed 22 months ago by jonasr

  • Resolution fixed deleted
  • Status changed from closed to reopened

Here's a patch for wxDateSpan::GetTotalMonths().

But starting to use this function in my app, I'm embarrased to say that my previous patch had a couple of bugs in it. Turns out that the test I added were to trivial. Dates are tricky.
Attached is a patch that fixes wxDateTime::DiffAsDateSpan, plus more extensive tests.

Hope it's ok to add 2 patches to an old ticket like this.

Regards
Jonas

Changed 22 months ago by jonasr

Changed 22 months ago by jonasr

comment:9 Changed 22 months ago by vadz

If it helps, it's rare for me to write any non trivial code working with dates without bugs too. The best is to generate a lot of random data (there are small Python scripts embedded into the test, e.g. see source:wxWidgets/trunk/tests/datetime/datetimetest.cpp#L383) and check that everything works correctly with it.

Will apply the fixes soon, thanks again!

comment:10 Changed 22 months ago by VZ

(In [72615]) Add wxDateSpan::GetTotalMonths() method.

This is similar to the existing GetTotalDays() and counts both months and
years.

See #14704.

comment:11 Changed 22 months ago by VZ

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [72616]) Fix bugs in the recently added wxDateTime::DiffAsDateSpan().

Correct the test for negative spans less than a month and use the correct
month for computing the number of days in it.

Also add unit tests for problematic cases.

Closes #14704.

Note: See TracTickets for help on using tickets.