search

Kotlin / kotlin-koans

Kotlin workshop
MIT License
2.6k stars 1.41k forks source link

Q35 Reinstating removed tests, they were correct but... #83

Closed jaymoid closed 7 years ago

jaymoid commented 7 years ago

I think the logic for calculating dates from millis and vice versa was incorrect. Java.util.Calendar uses zero based months to keep you on your toes! Also Calendar.getInstance() will get the default Locale (and also timezone) so the toMillis conversions it produces will be adjusted accordingly.

For example, Dates in December don't work with the old code! e.g. MyDate(2014, 12, 13) would create a date of: "2015-01-13" (Jan 13th 2015).

The orignal asserts in the tests were removed here: https://github.com/Kotlin/kotlin-koans/commit/fdc9d249005cf68fd2f9326356aef534edbc648d, because they were not reliable because of the above reasons, but I think their original intentions remain valid, so I have reinstated them and amended the calendar logic to be compatible with zero-based months and always use UTC.

The time in millis in the previous tests had an offset included, so when converted to UTC: 1392220800000 = 2014-02-12T16:00

So I have changed it to: 1389571200000 = 2014-01-13T00:00

This should yield succesfull test results when ran in different locales.

svtk commented 7 years ago

Implementations of 'toMillis' and 'toDate' are not the core of this task, so I'd avoid introducing any additional complexity to them.

jaymoid commented 7 years ago

Understood, however be aware that your MyDate class will still create confusing/incorrect dates without correct (yet peculiar) use of the java.util.Calendar class:

MyDate(2014, 12, 13) would create a date of: "2015-01-13" (Jan 13th 2015).

svtk commented 7 years ago

Thanks for pointing this out! I've added the explicit comment to the tests that "Month numbering starts with 0" to avoid any confusion. I don't see a value in changing the implementation because the exact dates are used in tests only.