Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

String Manipulation and Comparison Operations #341

Closed
wants to merge 2 commits into from

Conversation

tonyta
Copy link
Contributor

@tonyta tonyta commented May 12, 2016

Hi again!

I thought I'd suggest some changes after reading through this beautiful source!

Changes:

  1. Refactor some string manipulations so they are more performant (up to 3-4x faster) and more concise.
  2. Swapped around some comparisons to be more conventional: with the (more) constant object on the right of the operator.

Hope this is helpful! 😸

@@ -68,13 +68,13 @@ def perform(request, response)
# Check if we reached max amount of redirect hops
# @return [Boolean]
def too_many_hops?
1 <= @max_hops && @max_hops < @visited.count
@max_hops > 0 && @visited.count > @max_hops
Copy link
Contributor

Choose a reason for hiding this comment

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

I think @visited.size or @visited.length would be more descriptive here.

@ixti
Copy link
Member

ixti commented May 12, 2016

Agree with #1 - I like what you've did to content type parsing and will cherry-pick it.
But I have pretty strong objections against second one. :D

@@ -43,7 +43,7 @@ def coerce(object)
# @param [#to_s] str
# @return [Symbol]
def symbolize(str)
str.to_s.downcase.tr("-", " ").gsub(/[^a-z ]/, "").gsub(/\s+/, "_").to_sym
str.to_s.downcase.tr("- ", "_").delete("^a-z_").to_sym
Copy link
Member

Choose a reason for hiding this comment

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

[1] pry(main)> "Bad  request".downcase.tr("- ", "_").delete("^a-z_").to_sym
=> :bad__request
[2] pry(main)> "Bad  request".downcase.tr("-", " ").gsub(/[^a-z ]/, "").gsub(/\s+/, "_").to_sym
=> :bad_request

@ixti
Copy link
Member

ixti commented May 12, 2016

Partially merged in 015a707

@ixti ixti closed this May 12, 2016
@tonyta
Copy link
Contributor Author

tonyta commented May 12, 2016

@ixti Thanks for the quick feedback. I'm just curious, what are your objections to swapping the variables?

@ixti
Copy link
Member

ixti commented May 12, 2016

Well, mostly pretty subjective personal vision of beauty :D I prefer to see constants on the left of expression. I guess this is actually the only reason :D after all...

jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Oct 18, 2016
## 2.0.3 (2016-08-03)

* [#365](httprb/http#365)
  Add `HTTP::Response#content_length`
  ([@janko-m])

* [#335](httprb/http#335),
  [#360](httprb/http#360)
  Set `Content-Length: 0` header for `nil` bodies.
  ([@britishtea])


## 2.0.2 (2016-06-24)

* [#353](httprb/http#353)
  Avoid a dependency cycle between Client and Connection classes.
  ([@jhbabon])


## 2.0.1 (2016-05-12)

* [#341](httprb/http#341)
  Refactor some string manipulations so they are more performant
  (up to 3-4x faster) and more concise.
  ([@tonyta])

* [#339](httprb/http#341)
  Always use byte methods when writing/slicing the write buffer.
  ([@zanker])


## 2.0.0 (2016-04-23)

* [#333](httprb/http#333)
  Fix HTTPS request headline when sent via proxy.
  ([@Connorhd])

* [#331](httprb/http#331)
  Add `#informational?`, `#success?`, `#redirect?`, `#client_error?` and
  `#server_error?` helpers to `Response::Status`.
  ([@mwitek])

* [#330](httprb/http#330)
  Support custom CONNECT headers (request/response) during HTTPS proxy requests.
  ([@smudge])

* [#319](httprb/http#319)
  Drop Ruby 1.9.x support.
  ([@ixti])


## 1.0.4 (2016-03-19)

* [#320](httprb/http#320)
  Fix timeout regression.
  ([@tarcieri])


## 1.0.3 (2016-03-16)

* [#314](httprb/http#314)
  Validate charset before forcing encoding.
  ([@kylekyle])

* [#318](httprb/http#318)
  Remove redundant string allocations upon header names normalization.
  ([@ixti])
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants