Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Small simplification #157

Closed
wants to merge 1 commit into from
Closed

Small simplification #157

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 10 additions & 13 deletions src/ServerRequestFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,21 +198,18 @@ public static function marshalHeaders(array $server)
{
$headers = [];
foreach ($server as $key => $value) {
if ($value && strpos($key, 'HTTP_') === 0) {
$name = strtr(substr($key, 5), '_', ' ');
$name = strtr(ucwords(strtolower($name)), ' ', '-');
if ($value) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code has a much higher cyclomatic complexity, and is also harder to understand, IMO

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read it in following way:

  • we are only interested if there is some value
  • then we are curious what key we have
    • if key starts with HTTP_ - drop prefix
    • if key starts with CONTENT_ - take it as is
    • ignore key otherwise
  • normalize key (common for both cases for simplicity)
    • replace _ with - in key
    • make key lowercase

Added cyclomatic complexity is only because we've decoupled common operations for HTTP_* and CONTENT_* keys, otherwise it is the same. I thought there is no reason to duplicate 3 exactly the same lines in 2 neighbor blocks.

if (strpos($key, 'HTTP_') === 0) {
$name = substr($key, 5);
} elseif (strpos($key, 'CONTENT_') === 0) {
$name = $key;
} else {
continue;
}

$name = strtr($name, '_', '-');
$name = strtolower($name);

$headers[$name] = $value;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this just dropped? Was there no coverage for it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you analyze it correctly - it basically does nothing.
First we cut Content- at the beginning, then add it unconditionally (why?). After this we check whether remainder is equal to MD5 and if so - pass it directly, otherwise we make reminder lowercase and than make first character uppercase. After all manipulations we make it lowercase (which basically neglects everything from previous sentence).
So:

  • cutting CONTENT_ and replacing with Content- seems reasonable, but can be just replaced with strtr() (the real difference in practice is just one symbol) in order to keep is homogeneous with previous block of code
  • all the game with repeated upper/lowercasing doesn't make any sense

As far as I saw there are test cases for all situations touched by these changes.

continue;
}

if ($value && strpos($key, 'CONTENT_') === 0) {
$name = substr($key, 8); // Content-
$name = 'Content-' . (($name == 'MD5') ? $name : ucfirst(strtolower($name)));
$name = strtolower($name);
$headers[$name] = $value;
continue;
}
}

Expand Down