fix: Improve PHP syntax

[smnandre]

Another set of small improvments:

• Use=== over ==
• Use strict types over isset or empty
• Use spreads over array_merge (better performance)
• Use multiline docblocks over inlined

[meyfa]

• === over == is a welcome change, so are the docblocks and spread syntax.
• Please avoid Yoda conditions, i.e., prefer $params === null over null === $params.
• I would prefer if we continued using empty($array) instead of $array === []. In many languages this would perform an identity compare and would always return false, so having this syntax may be confusing DX. Also I did some microbenchmarking comparing the two options and empty($array) is consistently 2.34x faster.

[smnandre]

Please avoid Yoda conditions, i.e., prefer $params === null over null === $params.

Oops sorry, it's an habit. Would you agree if i add a phpcs rule to enforce "no yoda condition" ?

I would prefer if we continued using empty($array) instead of $array === [].

Then i'll change back :)

In many languages this would perform an identity compare and would always return false, so having this syntax may be confusing DX.

In PHP any empty array === [], but i understand what you mean for people using often other language. The original goal here was to enfore strict types (stating in those example this value cannot be a bool, null, empty strings etc)

Also I did some microbenchmarking comparing the two options and empty($array) is consistently 2.34x faster.

That in fact is really surprising to me... i made two small benchmark here so you can see

TL;DR; they are absolutely identical

Mulitple arrays (empty, one element, multipes)

• https://3v4l.org/25ueK/perf (empty with multiple arrays)
• https://3v4l.org/CcK9L/perf ([] === with multiple arrays)

Both produce the same exact code after the same number of ops

number of ops:  33
compiled vars:  !0 = $foo, !1 = $foo2, !2 = $foo3, !3 = $foo4

With a single empty array

• https://3v4l.org/A6C5M/perf (empty with one array)
• https://3v4l.org/NGLtV/perf ([] === with one array)

There again you can see the compiled code is the same (same number of ops)

number of ops:  5
compiled vars:  !0 = $a

So neither of them has a technical / performance one here :)

But it's your lib, so it's your code style and i'll happily use the CS you want 😃

[smnandre]

Apply your feedback and rebased :)

[meyfa]

Would you agree if i add a phpcs rule to enforce "no yoda condition" ?

Absolutely, if you know how 😅 I found phpcs weird to work with and the available rules kind of limited. Feel free to suggest improvements to our rules as you see fit. (Preferably in a new PR)

[meyfa]

Thank you for benchmarking as well, the site you linked is certainly better suited than what I used when creating mine, and the VLD tab is nice. However, I think your benchmarks are flawed since they execute only 4 comparisons, leading to no observable time difference since the overhead of running the overall PHP script is too large. I adapted your benchmarks to run the comparisons for many more iterations and there you can clearly see a time difference:

$array === []: https://3v4l.org/2qHoa/perf

• User time: 0.262 seconds

empty($array): https://3v4l.org/vBDue/perf

• User time: 0.187 seconds (1.4x faster)

The difference isn't as pronounced here as in my previous benchmark, and in general, both operations are quite fast. This probably borders on unnecessary micro-optimization in any case.

By the way, looking at the VLD tab, there are different instructions used here:

• $array === []:

13        FETCH_DIM_R                                      ~16     !4, ~15
14        IS_IDENTICAL                                             ~16, <array>

• empty($array):

13        ISSET_ISEMPTY_DIM_OBJ                         1          !4, ~15

So in fact, empty() uses one fewer instruction.

Again, it likely doesn't matter much and I agree with you that the strictness of === [] has some advantages as well. However, I still prefer empty() in this case, mostly for the syntax/DX reasons.

[meyfa]

Thank you for bearing with me throughout this back-and-forth, and sorry for the many delays.

Once the 0 vs. 0.0 thing is resolved this is good to go from my side.

[meyfa]

One last thing, just as a note to why I keep changing the PR titles: I'm trying to establish Conventional Commits throughout this project, which requires a title of the form <type>: <description> where <type> can be fix/feat/chore/docs etc. and <description> should start with an imperative verb.

[smnandre]

Would you agree if i add a phpcs rule to enforce "no yoda condition" ?

Absolutely, if you know how 😅 I found phpcs weird to work with and the available rules kind of limited. Feel free to suggest improvements to our rules as you see fit.

Forgot you use PHPCS ... i'll look soon to see how to do this :)

(Preferably in a new PR)

:)

[smnandre]

One last thing, just as a note to why I keep changing the PR titles: I'm trying to establish Conventional Commits throughout this project, which requires a title of the form <type>: <description> where <type> can be fix/feat/chore/docs etc. and <description> should start with an imperative verb.

Oops sorry. I promise i'll try to think about it.... 🤝

[smnandre]

Thank you for benchmarking as well, the site you linked is certainly better suited than what I used when creating mine, and the VLD tab is nice. However, I think your benchmarks are flawed since they execute only 4 comparisons, leading to no observable time difference since the overhead of running the overall PHP script is too large. I adapted your benchmarks to run the comparisons for many more iterations and there you can clearly see a time difference:

$array === []: https://3v4l.org/2qHoa/perf

* User time: 0.262 seconds

empty($array): https://3v4l.org/vBDue/perf

* User time: 0.187 seconds (1.4x faster)

The difference isn't as pronounced here as in my previous benchmark, and in general, both operations are quite fast. This probably borders on unnecessary micro-optimization in any case.

By the way, looking at the VLD tab, there are different instructions used here:

* `$array === []`:

13        FETCH_DIM_R                                      ~16     !4, ~15
14        IS_IDENTICAL                                             ~16, <array>

* `empty($array)`:

13        ISSET_ISEMPTY_DIM_OBJ                         1          !4, ~15

So in fact, empty() uses one fewer instruction.

Again, it likely doesn't matter much and I agree with you that the strictness of === [] has some advantages as well. However, I still prefer empty() in this case, mostly for the syntax/DX reasons.

What an objective and instructive answer, thank you!

No problem for empty as i said it's your repo :))

[smnandre]

Absolutely, if you know how 😅 I found phpcs weird to work with and the available rules kind of limited. Feel free to suggest improvements to our rules as you see fit. (Preferably in a new PR)

Would you want to switch to Php-Cs-Fixer ?

[meyfa]

Would you want to switch to Php-Cs-Fixer ?

Yes, that looks like a good alternative!

[smnandre]

Ok i'll see what i can do :))

Could you make me a list (if you have it in mind) of code-style rules you're attached to ? :)

[meyfa]

@smnandre See #229