-
Notifications
You must be signed in to change notification settings - Fork 85
Small optimalizations with count and strlen #129
Conversation
src/Header/ContentType.php
Outdated
| $header = new static($value, trim($mediaType)); | ||
|
|
||
| if (count($parts) > 0) { | ||
| if (!empty($parts)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space after not operator
| $explode = explode('=', $param, 2); | ||
|
|
||
| if (count($explode) === 2) { | ||
| if (isset($explode[1])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should check that key 0 exists, and key 2 doesn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The explode operation was given a limit of 2, so there's no chance a key 2 will ever exist.
| if (strlen($a->raw) == strlen($b->raw)) { | ||
| $aLength = strlen($a->raw); | ||
| $bLength = strlen($b->raw); | ||
| if ($aLength === $bLength) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spaceship operator: only case where I ever suggested its usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't use it here; this component still supports PHP 5.6.
|
|
||
| if (count($parts) > 0) { | ||
| if (! empty($parts)) { | ||
| $parameters = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty() not needed: the ! is sufficient
| foreach ($nvPairs as $nvPair) { | ||
| $parts = explode('=', $nvPair, 2); | ||
| if (count($parts) != 2) { | ||
| if (! isset($parts[1])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should check for key 2. Test needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind, explode is limited to 2 elements
| } elseif ($value instanceof Header\HeaderInterface) { | ||
| continue; | ||
| } | ||
| if (is_array($value)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move contents of this block to a private method
| if (empty($this->content)) { | ||
| $requestBody = file_get_contents('php://input'); | ||
| if (strlen($requestBody) > 0) { | ||
| if (! empty($requestBody)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty() not needed: ! is sufficient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for bool comparison usage, with if ($requestBody) { should be enough
|
Closing, as the author has not incorporated any of the requested changes. |
Something more to @samsonasik pr in #125