kotlinx-datetime icon indicating copy to clipboard operation
kotlinx-datetime copied to clipboard

Simplify LocalDate(Time) constructor parameter names

Open fluidsonic opened this issue 4 years ago • 7 comments
trafficstars

The parameter names are very weird for day-to-day use:

LocalDateTime(year = 2021, monthNumber = 1, dayOfMonth = 1, hour = 12)

I don't care that month is a number. That's what we have a type system for. OfMonth for day is also redundant as that's obvious from the context.

https://github.com/Kotlin/kotlinx-datetime/blob/3af71d2e874592fc70282de441a09d024dcefb54/core/common/src/LocalDateTime.kt#L50

fluidsonic avatar Dec 15 '20 12:12 fluidsonic

These parameters are named this way, so that one can get back the values passed to the constructor from the same-named properties of the constructed object.

ilya-g avatar Dec 16 '20 12:12 ilya-g

That property dayOfMonth should probably also simply be called day.

Kotlin tries to be pragmatic for the majority of use cases. The vast majority when working with dates is using the day of month. Everybody and their mother knows what a day refers to when dealing with a year/month/day date (+ time). dayOfWeek and dayOfYear is derived information and the latter used very rarely in my observation. dayOfMonth is overly specific. With auto-completion you also clearly see that there's day, dayOfWeek and dayOfYear so there's little room for error.

So, if Kotlin is a language optimized for general application development and the majority of use cases, day should be totally sufficient in both cases, constructor and property.

fluidsonic avatar Dec 16 '20 14:12 fluidsonic

Btw, the compiler makes things more confusing. I've provided year, month and day all as Int and with natural names. The compiler complains that I have to provide three Ints. But I did.

image (I've also just created KT-43960 for that.)

I also think that the names being more natural overweighs the benefit of having monthNumber symmetric with the property. Nobody says "month number" in day-to-day life, not even a developer. As for dayOfMonth see my previous reply.

fluidsonic avatar Dec 16 '20 14:12 fluidsonic

You make a good point about dayOfMonth. I agree, nobody will be confused about the meaning of LocalDateTime.day.

Regarding monthNumber, Month is a nice, well-typed, unambiguous thing. monthNumber: Int, on the other hand, is not. Unfortunately, many systems use zero-based numbering for months, even while using one-based numbering for days. JavaScript comes to mind:

new Date(2020, 1, 1) Date Sat Feb 01 2020 00:00:00 GMT+0100 (Central European Standard Time)

So, it makes sense for LocalDate(Time)? to highlight an area that would give potentially misleading results by using a longer name. "monthNumber? What kind of a number? The usual kind, 1..12, or zero-based?" If we manage to provoke a question of this kind, we have won.

In your example (which, I assume, is simplified, but nonetheless), Month.DECEMBER is, I think, a better choice, which should be more obvious with a less prominent month number, like 5 or 10.

WDYT?

dkhalanskyjb avatar Jul 26 '22 12:07 dkhalanskyjb

I believe that numbering months from zero is an abomination and should not be considered a normal practice that we take into account when naming the corresponding parameter/property.

ilya-g avatar Jul 26 '22 15:07 ilya-g

What is an abomination is using the day field to store the index of the week in the month (https://docs.microsoft.com/en-us/windows/win32/api/timezoneapi/ns-timezoneapi-time_zone_information, see the StandardDate section). I agree we shouldn't provide any support for that kind of abominations. Zero-based months are fine by comparison and quite ubiquitous: https://grep.app/search?q=JANUARY%20%3D%200 https://grep.app/search?q=%22JANUARY%22%3A%200&words=true APIs are messy and often done as an afterthought (producing such abominations), so data plumbing benefits from getting rid of raw numbers as soon as possible in favor of typed arguments.

dkhalanskyjb avatar Jul 27 '22 09:07 dkhalanskyjb

Here's the proposal:

  • Rename dayOfMonth to day everywhere. It's consistent with how we have LocalTime.nanosecond and don't call it nanosecondOfSecond (as opposed to nanosecondOfDay, for example).
  • Remove monthNumber from LocalDate: it's easy to access it as month.number, which also guides people to just use month. There's some code out there like Month(date.monthNumber), for example.
  • Rename the monthNumber constructor parameter to just month.
  • Keep monthNumber in DateTimeComponents (as it's significantly different from month there and has its own uses).
  • Keep monthNumber in the format builders, as having month + monthName is less clear than monthNumber + monthName.

dkhalanskyjb avatar Mar 11 '24 13:03 dkhalanskyjb