Open fluidsonic opened 3 years ago
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.
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.
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 Int
s. But I did.
(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.
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?
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.
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.
Here's the proposal:
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).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.monthNumber
constructor parameter to just month
.monthNumber
in DateTimeComponents
(as it's significantly different from month
there and has its own uses).monthNumber
in the format builders, as having month
+ monthName
is less clear than monthNumber
+ monthName
.
The parameter names are very weird for day-to-day use:
I don't care that
month
is a number. That's what we have a type system for.OfMonth
forday
is also redundant as that's obvious from the context.https://github.com/Kotlin/kotlinx-datetime/blob/3af71d2e874592fc70282de441a09d024dcefb54/core/common/src/LocalDateTime.kt#L50