cakephp / chronos

A standalone DateTime library originally based off of Carbon
http://book.cakephp.org/chronos
MIT License
1.34k stars 61 forks source link

Add third Parameter $others to farthest and closest Methods #422

Closed brenoroosevelt closed 9 months ago

brenoroosevelt commented 10 months ago

Currently, the farthest and closest methods in the Chronos class accept only two Chronos objects as parameters, limiting their functionality. There are use cases where it would be beneficial to calculate the farthest or closest Chronos object among three or more.

Proposal:

This pull request aims to enhance the flexibility of the farthest and closest methods by adding a third parameter named $others. This parameter will accept an arbitrary number of Chronos objects, allowing users to calculate the farthest or closest object among multiple instances.

Changes Made:

public function farthest(Chronos $first, Chronos $second, Chronos ...$others): Chronos;
public function closest(Chronos $first, Chronos $second, Chronos ...$others): Chronos;

This modification maintains backward compatibility, as the methods can still be used with only two parameters if needed. The code has been tested to ensure proper functionality.

othercorey commented 10 months ago

I think splitting up an array to 3 parameters would be awkward. Maybe we should support a single variable length parameter.

brenoroosevelt commented 10 months ago

I think splitting up an array to 3 parameters would be awkward. Maybe we should support a single variable length parameter.

Thank you for your comments, but I have a different perspective that I'd like to share.

Adding a third parameter, the primary intent of the function remains clear: comparing at least two parameters. My proposal involves adding a third parameter to the closest and farthest methods while maintaining compatibility with the original method signature. This addition ensures that users can maintain compatibility by using either two positional arguments or named parameters when calling the closest method.

IMHO, a single variable-length parameter (array) has disadvantages:

othercorey commented 10 months ago

I'm not sure we need to require 2 items in the array and named parameters don't mean anything in this case.

brenoroosevelt commented 10 months ago

Since PHP 8, when a developer uses named parameters, a single name change (in params) makes method unusable. It's a BC issue. It appears that the primary intent of the method is to require at least 2 elements for comparison. Allowing 0 to N elements implies the need for validations to either throw an exception when 0 elements are provided or return null. In any case, it results BC issue. Furthermore, allowing 0..1 method semantics seems to make no sense.

othercorey commented 9 months ago

This looks good. Users will have to unroll an array anyway and this works with that solution and is BC.

brenoroosevelt commented 9 months ago

@othercorey, to support a single variable-length parameter while maintaining backward compatibility, it requires the addition of some additional methods. if this is the path forward, I can make the necessary changes.

    public function farthest(Chronos $first, Chronos $second, Chronos ...$others): Chronos
    {
        return $this->findFarthestOrFail([$first, $second, ...$others]);
    }

    /**
     * Find the farthest date from the instance.
     * @param iterable<\Cake\Chronos\Chronos> $values
     * @return ?self
     */
    public function findFarthest(iterable $values): ?Chronos
    {
        $farthest = null;
        $farthestDiff = null;
        foreach ($values as $value) {
            $otherDiff = $this->diffInSeconds($value);
            if (!$farthest || $otherDiff > $farthestDiff) {
                $farthest = $value;
                $farthestDiff = $otherDiff;
            }
        }

        return $farthest;
    }

    /**
     * Find the farthest date from the instance or fail
     * @param iterable<\Cake\Chronos\Chronos> $values
     * @return self
     * @throws \InvalidArgumentException
     */
    public function findFarthestOrFail(iterable $values): Chronos
    {
        return $this->findFarthest($values) ?? throw new InvalidArgumentException(
            'It\'s not possible to find the farthest value as the collection is empty'
        );
    }