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

Refactor array_map and array_filter #102

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HPWebdeveloper
Copy link

This Pull Request introduces a refactoring of the getPugEngine method within the ServiceProvider. The primary objectives are to simplify the logic for enhanced readability and to improve overall performance.

return in_array('pug', $engines);
}));
foreach ($extensions as $key => $engine) {
if (strpos($engine, 'pug') !== false) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not equivalent as it searches for any pug sub-string not delimited by . separator.

}));
foreach ($extensions as $key => $engine) {
if (strpos($engine, 'pug') !== false) {
$filteredExtensions[] = ".$key";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Current code uses the value, not the key.

@kylekatarnls
Copy link
Collaborator

kylekatarnls commented Dec 17, 2023

array_map and array_filter was slow in the past (when they were actually PHP-coded functions), but have been rewritten in C and since they are actually faster than loops (and creating intermediate variables) in many situations (https://medium.com/@visheshahuja/php-array-map-vs-foreach-loop-18488c95aa18)

So unless some benchmark using recent PHP version, I wouldn't bet performance is a win here.

Also codes before and after here are not equivalent, it might break some untested cases, the readability improvement does not sound worthy to me as a first sight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants