Small optimizations #267

snapshotpl · 2017-08-24T22:19:25Z

No description provided.

Ocramius · 2017-08-25T03:00:08Z

src/CallbackStream.php

@@ -172,10 +172,10 @@ public function getMetadata($key = null)
            return $metadata;
        }

-        if (! array_key_exists($key, $metadata)) {
-            return null;
+        if (isset($metadata[$key])) {


return $metadata[$key] ?? null;

Good idea but after drop php5 ;(

Wha?! This is for an improvement: just drop it already

But is it not better to merge it as it to current php5 supported line and then, drop php5 and apply you have suggested?

If we can optimize for PHP 5.6 today, we can make it even better for the next major release later. I'm fine with this for now.

Ocramius · 2017-08-25T03:01:20Z

src/RelativeStream.php

@@ -107,7 +107,7 @@ public function isSeekable()
     */
    public function seek($offset, $whence = SEEK_SET)
    {
-        if ($whence == SEEK_SET) {
+        if ($whence === SEEK_SET) {


Can you use the FQN for this constant?

I'm not sure what are you meant

\SEEK_SET

Ocramius · 2017-08-25T03:02:24Z

src/ServerRequestFactory.php

@@ -60,7 +60,7 @@ public static function fromGlobals(
        $files   = static::normalizeFiles($files ?: $_FILES);
        $headers = static::marshalHeaders($server);

-        if (null === $cookies && array_key_exists('cookie', $headers)) {
+        if (null === $cookies && isset($headers['cookie'])) {


You need to introduce tests around null values to avoid regressions here

I think it's imposible to write this test becase $headers comes from private function which alware return strings pair array.

No need then: nullability was just an assumption on my end.

Ocramius · 2017-08-25T03:02:33Z

src/ServerRequestFactory.php

@@ -206,7 +206,7 @@ public static function marshalHeaders(array $server)

                // We will not overwrite existing variables with the
                // prefixed versions, though
-                if (array_key_exists($key, $server)) {
+                if (isset($server[$key])) {
                    continue;


You need to introduce tests around null values to avoid regressions here

Agreed, particularly as the values may resolve to a false (e.g., 0, ''). The reason array_key_exists() was used was to allow any value, regardless of whether or not it evaluated as truthy.

Ocramius · 2017-08-25T03:03:01Z

src/Stream.php

@@ -285,11 +285,11 @@ public function getMetadata($key = null)
        }

        $metadata = stream_get_meta_data($this->resource);
-        if (! array_key_exists($key, $metadata)) {
-            return null;
+        if (isset($metadata[$key])) {


Use ?? here too

If you target PHP 7.1+, you'll need to update the composer.json, rebase against develop, and change the branch this is against.

Ocramius · 2017-08-25T03:04:15Z

src/Uri.php

@@ -641,7 +641,7 @@ private function filterQuery($query)
    private function splitQueryValue($value)
    {
        $data = explode('=', $value, 2);
-        if (1 === count($data)) {
+        if (! isset($data[1])) {


This check is not equivalent to the previous: you need to verify that the key 0 exists too

@Ocramius explode() should always has key 0 ? https://3v4l.org/viNZT

@Ocramius agree with @samsonasik

Yup, misread

snapshotpl · 2017-09-05T21:21:38Z

@Ocramius all remarks applied in code or I miss something?

michalbundyra · 2017-09-05T22:11:56Z

@snapshotpl tests are failing. Why?

michalbundyra · 2017-09-06T08:08:09Z

src/RelativeStream.php

    {
-        if ($whence == SEEK_SET) {
+        if ($whence === \SEEK_SET) {


I would go with import the constant as we do in other places for internal functions:

use const SEEK_SET;

weierophinney

I'm fine with the majority of changes, but I do question whether some of these micro-optimizations make sense, as they make the code less readable.

weierophinney · 2017-09-11T19:54:49Z

src/ServerRequestFactory.php

@@ -206,7 +206,7 @@ public static function marshalHeaders(array $server)

                // We will not overwrite existing variables with the
                // prefixed versions, though
-                if (array_key_exists($key, $server)) {
+                if (isset($server[$key])) {
                    continue;


Agreed, particularly as the values may resolve to a false (e.g., 0, ''). The reason array_key_exists() was used was to allow any value, regardless of whether or not it evaluated as truthy.

weierophinney · 2017-09-11T19:56:03Z

src/Stream.php

+            || strpos($mode, 'w') !== false
+            || strpos($mode, 'c') !== false
+            || strpos($mode, 'a') !== false
+            || strpos($mode, '+') !== false


How is this change "better" or an "optimization", exactly? I find it less readable.

weierophinney · 2017-09-11T19:57:02Z

src/Stream.php

@@ -234,7 +236,7 @@ public function isReadable()
        $meta = stream_get_meta_data($this->resource);
        $mode = $meta['mode'];

-        return (strstr($mode, 'r') || strstr($mode, '+'));
+        return (strpos($mode, 'r') !== false || strpos($mode, '+') !== false);


Same comment as above regarding strstr vs strpos. I recognize that strpos is marginally faster, but most folks agree it's a micro-optimization at best. If using it sacrifices readability, I'm not sure the change is useful.

weierophinney · 2017-09-11T19:58:00Z

src/Stream.php

@@ -285,11 +285,11 @@ public function getMetadata($key = null)
        }

        $metadata = stream_get_meta_data($this->resource);
-        if (! array_key_exists($key, $metadata)) {
-            return null;
+        if (isset($metadata[$key])) {


If you target PHP 7.1+, you'll need to update the composer.json, rebase against develop, and change the branch this is against.

weierophinney · 2018-05-29T15:52:34Z

Closing; many changes were requested, but none provided by the author.

If you or anyone else reading want to see these changes, please submit a new PR incorporating them, along with the requested feedback.

Thanks!

snapshotpl added 2 commits August 25, 2017 00:18

Small optimizations

d1035e9

CS fix

432959b

Ocramius suggested changes Aug 25, 2017

View reviewed changes

snapshotpl added 2 commits August 29, 2017 23:59

Use FQN

741ffc9

Add test case

78372ea

michalbundyra reviewed Sep 6, 2017

View reviewed changes

snapshotpl added 2 commits September 11, 2017 21:20

Fix test

d954049

use const

a481284

weierophinney suggested changes Sep 11, 2017

View reviewed changes

weierophinney closed this May 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Small optimizations #267

Small optimizations #267

snapshotpl commented Aug 24, 2017

Ocramius Aug 25, 2017

snapshotpl Aug 25, 2017

Ocramius Aug 30, 2017

snapshotpl Aug 30, 2017

weierophinney Oct 12, 2017

Ocramius Aug 25, 2017

snapshotpl Aug 25, 2017

Ocramius Aug 25, 2017

Ocramius Aug 25, 2017

snapshotpl Aug 29, 2017

Ocramius Aug 30, 2017

Ocramius Aug 25, 2017

weierophinney Sep 11, 2017

Ocramius Aug 25, 2017

weierophinney Sep 11, 2017

Ocramius Aug 25, 2017

samsonasik Aug 26, 2017

snapshotpl Aug 29, 2017

Ocramius Aug 30, 2017

snapshotpl commented Sep 5, 2017

michalbundyra commented Sep 5, 2017

michalbundyra Sep 6, 2017

weierophinney left a comment

weierophinney Sep 11, 2017

weierophinney Sep 11, 2017

weierophinney Sep 11, 2017

weierophinney Sep 11, 2017

weierophinney commented May 29, 2018

Small optimizations #267

Small optimizations #267

Conversation

snapshotpl commented Aug 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snapshotpl commented Sep 5, 2017

michalbundyra commented Sep 5, 2017

Choose a reason for hiding this comment

weierophinney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney commented May 29, 2018