Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Sep 22, 2023

Conversation

brenoroosevelt
Copy link
Contributor

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:

  • Added a third parameter ...$others to the farthest and closest methods in the Chronos and ChronosDate classes.
  • Updated the methods' logic to accommodate the new parameter.
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
Copy link
Member

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

@brenoroosevelt
Copy link
Contributor Author

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:

  • It doesn't maintain compatibility, especially due to named parameters, breaking the code closest(first: Chronos::now(), second: Chronos::now())
  • It requires additional validations to ensure the presence of at least 2 elements.
  • It doesn't clearly communicate the requirement of at least 2 elements for the method's semantics to make sense.
  • It lacks strong typing.

@othercorey
Copy link
Member

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

@brenoroosevelt
Copy link
Contributor Author

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.

@markstory markstory added this to the 3.x milestone Sep 19, 2023
@othercorey
Copy link
Member

othercorey commented Sep 19, 2023

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

@brenoroosevelt
Copy link
Contributor Author

@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'
        );
    }

@othercorey othercorey merged commit 478b801 into cakephp:3.x Sep 22, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants