10000 Consider replacing array_replace() with array union (+ operator) · Issue #7029 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Consider replacing array_replace() with array union (+ operator) #7029

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.

Sign up for GitHub

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

Jump to bottom
Closed
rybakit opened this issue Feb 8, 2013 · 12 comments
Closed

Consider replacing array_replace() with array union (+ operator) #7029

rybakit opened this issue Feb 8, 2013 · 12 comments
Labels
Feature

Comments

@rybakit
Copy link
Contributor
rybakit commented Feb 8, 2013

See discussion of #6995 (comment)
@vicb
Copy link
Contributor
vicb commented Feb 9, 2013

I also think we should promote array unions as a best practice. On top of that the code would be faster which is an other small benefit.

@dlsniper
Copy link
Contributor
dlsniper commented Feb 9, 2013

I'll make a PR for this soon, question is: on which branch should this go? 2.0, 2.1, 2.2, or master? I'd prefer 2.2 since it would benefit the upcoming release and it's not a breaking change/new feature.

@vicb any pointers?

@lyrixx
Copy link
Member
lyrixx commented Feb 9, 2013

What about php 5.3 ?

@fabpot
Copy link
Member
fabpot commented Feb 9, 2013

I'm against this change for many reasons:

  • we have better things to do
  • the speed difference does not matter
  • I don't want to encourage random changes for no good reasons
  • using + instead of array_replace makes the code less readable (just have a look at the code in the gist, the + version seems unintuitive to me)

@dlsniper
Copy link
Contributor
dlsniper commented Feb 9, 2013

@lyrixx what about it? It behaves the same in PHP 5.3+ (until 5.5 alpha4)

If @fabpot doesn't agree with this then I'll hold of with the PR.

Also, if you ever need to compare different functions speed/memory usage/opcodes/ outputs against multiple PHP versions you can use this site: http://3v4l.org/

@rybakit
Copy link
Contributor Author
rybakit commented Feb 9, 2013

@fabpot, your last statement is debatable. My original concern was not even about speed but readability, I always have to check the documentation to ensure how array_replace works, instead of +.

@lyrixx
Copy link
Member
lyrixx commented Feb 9, 2013

@dlsniper Ok. I just asked if php 5.3 has the same (speed) behavior than php 5.4.

@stof
Copy link
Member
stof commented Feb 11, 2013

@rybakit but when you see $a + $b, what is it ? array_replace or a sum ? you would have to check each time where the variables are coming from to understand what this code does.

@lyrixx what could cause a speed difference is xDebug which slows down functions (this is the reason for most of the difference between isset and array_key_exists too btw). and this is irrelevant in prod as xDebug is generally not installed there (at least if you care about such micro optimizations)

@vicb
Copy link
Contributor
vicb commented Feb 11, 2013

@stof I don't see what else than "union" a + between two array would mean ? And this is the first intend of the PR, makes thing clearer (vs array_replace[_recursive] + sometime a patched version).

If you read the history (link on top) you will see that xDebug is disabled and speed optimization is not the primary goal but just something that comes with.

@rybakit
Copy link
Contributor Author
rybakit commented Feb 11, 2013

@stof, actually $c = array_replace($a, $b); is not much cleaner:

  • What is $c? New array or Boolean, which indicates failed operation, like in array_walk()? Or maybe $c is a number of replaced keys?
  • Is $a passed by reference or not?
  • How many arguments can I pass to this function? 2 or more? Or maybe there is a nice 3d optional argument, like $preserve_keys = true?

Even if you know how array_replace works, you can't guarantee that $c is array, it can be null according to the documentation.

It's more like is_null($a) vs null === $a comparison. We can use is_null, but why should we if there is a built in operator?

@stof
Copy link
Member
stof commented Feb 11, 2013

@vicb @rybakit with $c = array_replace($a, $b);, you know that $a and $b are arrays and so it is an union. With $c = $a + $b, you have to check the place where the variables are coming from to know that they are arrays before knowing what + is doing.

@vicb
Copy link
Contributor
vicb commented Feb 11, 2013

usually $b is an "array()" definition, an $a is a parameter (options) myInit(array $options). Check the mentioned link for an example.

But I must admit that improving readability is subjective here (Fabien & you find "+" harder to read than array_replace). So maybe the best thing to do is to close this issue ?

@fabpot fabpot closed this as completed Feb 12, 2013
@rybakit rybakit mentioned this issue Aug 31, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Feature
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@fabpot @vicb @lyrixx @stof @rybakit @dlsniper
0