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

4.x - Eliminate a duplicate code via HOF #2809

Merged
merged 5 commits into from Aug 19, 2019

Conversation

mapogolions
Copy link
Contributor

  • reduce responsibility of some methods
  • move duplicate code into a separate private function
  • add more strict type hinting

@coveralls
Copy link

coveralls commented Aug 17, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 772c5ad on mapogolions:maintenance/duplicate-code into a139440 on slimphp:4.x.

Copy link
Member

@l0gicgate l0gicgate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks tidier for sure. @adriansuter please review before I merge.

@l0gicgate l0gicgate added this to the 4.2.0 milestone Aug 18, 2019
@l0gicgate l0gicgate changed the title Eliminate a duplicate code via HOF 4.x - Eliminate a duplicate code via HOF Aug 18, 2019
Copy link
Contributor

@adriansuter adriansuter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonderful work.

One minor change request and one question: Do you think it would make sense to take the Closures out of the resolveRoute and resolveMiddleware methods? Otherwise php has to create the Closure over and over again, whenever the middleware or route get resolved.

For example:

    protected function routePredicate($it): bool
    {
        return $it instanceof RequestHandlerInterface;
    }

    protected function middlewarePredicate($it): bool
    {
        return $it instanceof MiddlewareInterface;
    }

    /**
     * {@inheritdoc}
     */
    public function resolveRoute($toResolve): callable
    {
        return $this->resolveByPredicate($toResolve, [$this, 'routePredicate'], 'handle');
    }

    /**
     * {@inheritdoc}
     */
    public function resolveMiddleware($toResolve): callable
    {
        return $this->resolveByPredicate($toResolve, [$this, 'middlewarePredicate'], 'process');
    }

Of course the signature of resolveByPredicate would have to be adapted.

Slim/CallableResolver.php Outdated Show resolved Hide resolved
@mapogolions
Copy link
Contributor Author

mapogolions commented Aug 18, 2019

@adriansuter we were able to remove duplicate code, thanks to ideas from FP. In particular - higher order function. As you know, a function is treated like a Fist Class Citizen (as any variable). Creating a local function with every function call is a common practice. Other languages have more concise syntax, like it.map(x => x + 1)

@adriansuter
Copy link
Contributor

Of course. I am open to any solution. Other languages do not only have more concise syntax, but they often have compilers and optimizers too. I am not sure, whether PHP does recognize the Closure to be the same during a second call. And I don't know if there would be any negative performance impact. But I can imagine, there is. Not sure, how big though.

Nevertheless, writing the Closures in the two methods would have the advantage, that everything "needed" is written inside the methods. So that might be better for maintainability.

As I said, I don't actually have a strong opinion for one or the other solution. Both have their advantages and disadvantages.

@mapogolions
Copy link
Contributor Author

mapogolions commented Aug 18, 2019

Since I already made a commit that removes predicates from methods. Let's it be as it is. I think this is not so fundamental.

@l0gicgate l0gicgate merged commit 200e72a into slimphp:4.x Aug 19, 2019
@mapogolions mapogolions deleted the maintenance/duplicate-code branch August 19, 2019 04:37
@l0gicgate l0gicgate mentioned this pull request Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants