Resolve API inconsistencies regarding scalars versus objects

[joshdifabio]

There are some inconsistencies in the API, e.g.

class LocalDate
{
    public function getDayOfWeek(): DayOfWeek;

    public function getMonth(): int;
}

I understand why we've got getDay(): int and getYear(): int -- those values are naturally integer, and java.time makes the same judgement -- but what's the reason for treating getDayOfWeek() differently to getMonth()? These are naturally both enums.

[BenMorel]

You're right that these are inconsistencies in the API. I found the decisions quite hard to make at the time I wrote these.

As I see it now, I'd say that these should all return integers. An object can be explicitly requested by calling, for example, DayOfWeek::of($date->getDayOfWeek()) if needed.

What do you think?

And do you see other inconsistencies?

[joshdifabio]

I found the decisions quite hard to make at the time I wrote these.

I can imagine! This really is an excellent package though and it's clear that a lot of time, thought and skill has gone into it -- thank you so much for taking the time to create it and open source it.

Regarding the matter at hand, the first thing I'd say is that we can't possibly replicate the collective brainpower and empirical evidence that went into designing JSR 310 and so, where practical, I would suggest simply copying JSR 310's API. I think we are unlikely to make better subjective judgements than the original authors made, particularly because we have far less experience with these APIs than the authors of JSR 310 had (JSR 310 is primarily a formalisation of an API which was used by thousands of developers over a number of years before JSR 310 came along, as you likely already know).

Regarding java.time's API, values which are Enums are primarily passed around as Enums (Month and DayOfWeek), not scalars. So, all of the getMonth() and getDayOfWeek() methods return Enums, not ints. I would simply replicate this here. getYear() and getDayOfMonth() are different because these are not Enums.

In terms of practical benefits, and my personal opinion, passing integers around means we have to constantly validate values to check whether they are valid months/weekdays, which negates one of the benefits of this package. Value objects and enums can eliminate a lot of boilerplate. Therefore, the enums are more useful than bare integers, and so I would primarily use them where they're available. Furthermore, months and weekdays are not naturally numeric. We don't refer to month 7 or weekday 3, we say July and Tuesday, whereas we do refer to years and days of the month in numeric terms.

However, I would very much put my opinions as secondary to those of the original authors of JSR 310, which just happen to be the same in this instance.

And do you see other inconsistencies?

I haven't noticed anything else yet. Fantastic work, thank you again for using your time and skill to serve the open source community.

[BenMorel]

Regarding the matter at hand, the first thing I'd say is that we can't possibly replicate the collective brainpower and empirical evidence that went into designing JSR 310

This is very true, however PHP does not have the same functionalities as Java, so small differences are to be expected in some places. Also, for the record, I'm not against making deliberate opposite decisions, I did this for example in brick/money: JSR 354 has been a big source of inspiration and I took quite a lot of time to browse the spec and the reference implementation, but then I deliberately chose to not embed things such as rounding mode into the Money itself. Who made the best call? I still don't know, but I'm happy with the end result now, and the feedback I got so far was positive.

Furthermore, months and weekdays are not naturally numeric. We don't refer to month 7 or weekday 3, we say July and Tuesday, whereas we do refer to years and days of the month in numeric terms.

This is a very good point! 👍

I guess I remember what felt inconsistent to me at the time (and still does, to some extent), is the following:

• a LocalTime has 3 fields (ignore the nanos for now): hour, minutes and seconds
  • getHour() : int
  • getMinute() : int
  • getSecond() : int
• a LocalDate has three fields: year, month and day:
  • getYear() : int
  • getMonth() : Month
  • getDay() : int

It feels a bit wrong to me, that most fields are returned as integers, and one of them is returned as an enum (well, an object, as we don't have enums).

You could be surprised that you can't do:

sprintf('...', $date->getYear(), $date->getMonth(), $date->getDay());

Actually I can see that Java provides a getMonthValue() method, which fits the bill. As a consumer of the API though, I think it can be surprising to not get the integer value out of the getMonth() method, and have to use getMonthValue() instead.

This is just my gut feeling, of course, and you approach is most likely the correct one.

I would be happy to revisit the API for version 0.2; would you be willing to compare Brick\DateTime's API with Java's, and list all differences here?

At least we could see where exactly the APIs diverge, and this could help orient the decisions a lot.

[jkuchar]

I personaly prefer to have simple VO object types even when there is no logic on given object. It is then type-safe to pass them around. Question is - is there usecase for passing just Month around? If it is with year there is YearMonth object for that. And that makes much more sense in public API.

I'm not quite sure if it can be practical to have Second, Minute, Hour, Day, Month, Year as it will be quite messy to create these objects then. However they are not just an integers. They must be from domain of numbers - e.g. seconds must be 0-59, hour 0-23. Any ideas how to make API practical to use with these new objects?

[BenMorel]

Java does not have objects for Second, Minute, Hour, Day AFAIK. They've been explicitly introducing classes for just a handful of them: Year, Month, DayOfWeek.

The LocalDateTime object returns just two of them as non-integers: getMonth() and getDayOfWeek().

It may make sense as @joshdifabio explained very well:

We don't refer to month 7 or weekday 3, we say July and Tuesday, whereas we do refer to years and days of the month in numeric terms.

I have a little problem with getMonth() myself for the reasons explained above, but for getDayOfWeek(), it makes a lot of sense as there are several conventions, such as using 0 or 7 for Sunday. Returning an object removes all confusion.

[joshdifabio]

I think at this point it's all pretty subjective so I won't attempt to add anything else to the discussion beyond what I've already said.

I would be happy to revisit the API for version 0.2; would you be willing to compare Brick\DateTime's API with Java's, and list all differences here?

I don't think I will be able to commit the time to do this, sorry :(.

[BenMorel]

I'm still hesitating to change getMonth() to return a Month, I'm trying to find a use case where this would be desirable, and weighting this against everyday use of an int for the Month.

For example, this code from the tests:

        $this->compare([$year, $month, $day], [
            $date->getYear(),
            $date->getMonth(),
            $date->getDay()
        ]);

Would become:

        $this->compare([$year, $month, $day], [
            $date->getYear(),
            $date->getMonthValue(),
            $date->getDay()
        ]);

Which I find quite counter-intuitive.

And because we don't have enums in PHP, Month is a class instance, so you can't compare the value to constants directly. For example, in Java you would do:

if (date.getMonth() == Month.JANUARY) {

Where in PHP this becomes more verbose if we decide to return an object:

if ($date->getMonth()->is(Month::JANUARY)) {

Currently, we can do:

if ($date->getMonth() === Month::JANUARY) {

Which is cleaner IMO. At the same time, I agree that there is somewhat of an inconsistency to return DayOfWeek as an object, and month as an int.

I would be interested to hear a valid use case for using LocalDate::getMonth() : Month.

[BenMorel]

Actually there is no getDay() in Java, it's called getDayOfMonth(). So if we mimic their API, the example above would become:

        $this->compare([$year, $month, $day], [
            $date->getYear(),
            $date->getMonthValue(),
            $date->getDayOfMonth()
        ]);

Which is harder to read and memorize.

[joshdifabio]

For example, this code from the tests:

    $this->compare([$year, $month, $day], [
        $date->getYear(),
        $date->getMonth(),
        $date->getDay()
    ]);

I don't think this is a good example, because you would not typically hold separate references to a year, month and day of month if you were using this library properly; rather, you would use a LocalDate.

Month is useful when you are dealing with only a month, otherwise you would use LocalDate, YearMonth or MonthDay, so it would be best if we constrain our discussion to such cases. In these cases where we are dealing with only a month, we probably either have a function parameter or return type which is a value representing a month; depending on preference, the type of this value could be Month or int. If Month, we don't need to validate such values. Assuming we want to fail fast on errors, which we should, the month in int form potentially needs to be validated every time we see it.

function foo(Month $month) {
    // we know that $month is valid
} 

function foo(int $month) {
    // we do not know that $month is valid
} 

And because we don't have enums in PHP, Month is a class instance, so you can't compare the value to constants directly.

Enums are objects in Java too. Often these enum objects are constructed like this in PHP: Month::january() or Month::JANUARY().

Actually there is no getDay() in Java, it's called getDayOfMonth() ... Which is harder to read and memorize.

This is another case where you should have stuck with the Java API in my opinion. getDay is ambiguous. Sunday is also a day. For example, in JavaScript, getDay() refers to the day of week (MDN).

A lot of very smart people have already had these conversations.

[BenMorel]

I'll revisit this issue soon, in the light of enums now being available in PHP 8.1.

[gnutix]

@BenMorel I think this issue can now be closed, what do you think ?

[BenMorel]

@gnutix I think we can close this once getMonth() will return a Month enum.

As I see it:

• deprecation of getMonth(): int in favour of getMonthValue() in 0.6.0 (✅ done)
• removal of getMonth(): int in 0.7.0
• addition of getMonth(): Month in 0.8.0

[BenMorel]

Actually, I may go straight to changing the signature in 0.7.0. Either way it will throw an error if people didn't address the deprecation, so there may be little value in doing it in 2 steps. Thoughts?