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

Conversation

nazar-pc
Copy link

Original code seems over-engineered


if ($value && strpos($key, 'CONTENT_') === 0) {
$name = substr($key, 8); // Content-
$name = 'Content-' . (($name == 'MD5') ? $name : ucfirst(strtolower($name)));
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.

@weierophinney
Copy link
Member

I'm going to keep the code as-is, for the following reasons:

  • Having two separate blocks makes it quite clear what the two header sources are within $_SERVER. I personally feel this is important for developers trying to understand how $_SERVER works within the standard SAPI implementations.
  • Lower cyclomatic complexity.

@nazar-pc
Copy link
Author

I can refactor it to have 2 separate blocks, but leaving as-is means by having lower cyclomatic complexity there will be much more CPU cycles wasted for nothing

@weierophinney
Copy link
Member

there will be much more CPU cycles wasted for nothing

This code runs at the start of a request, and will be executed for only as many entries as are present in $_SERVER. The changes are a micro-optimization at best, and sacrifice clarity.

@nazar-pc
Copy link
Author

Final change from my side, refactored as promised: nazar-pc@c3cc4b4
If you really really think that clarity was sacrificed, then I have nothing to add.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants