Skip to content

Conversation

@KennedyTedesco
Copy link
Contributor

This removes the only compact() function call in Container and replaces it with a direct array, saving a function call.

@taylorotwell taylorotwell merged commit 493a46c into laravel:11.x Dec 8, 2024
40 checks passed
@KennedyTedesco KennedyTedesco deleted the container branch December 9, 2024 18:52
browner12 pushed a commit to browner12/framework that referenced this pull request Dec 10, 2024
@andrey-helldar
Copy link
Contributor

Benchmark::dd([
    'compact' => function () {
        $foo = 'foo';

        $item = compact('foo');

        return $item['foo'];
    },
    'array'   => function () {
        $foo = 'foo';

        $item = ['foo' => $foo];

        return $item['foo'];
    },
], 10000);
$ art foo
array:2 [
  "compact" => "0.001ms"
  "array" => "0.001ms"
]

The compact function reads easier ¯_(ツ)_/¯

@KennedyTedesco
Copy link
Contributor Author

KennedyTedesco commented Dec 11, 2024

The performance difference is negligible (even though the array literal requires fewer opcodes to be executed).

Overall, I have the opposite opinion: I always prefer the array literal version because:

  • It's more explicit and easier to read (in my opinion).
  • It requires fewer opcode operations.
  • It's more IDE-friendly for static analysis and AST tools like Rector etc (even though static analyzers can handle the compact function).

I'm 100% in favor of using the array literal version, especially in this case where only two values are being handled.

@andrey-helldar
Copy link
Contributor

In this case, the phrase “all felt-tip pens are different for taste and color” applies 🙂

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.

3 participants