Skip to content
Browse files

Load object/blank and make use of presence.

  • Loading branch information...
1 parent f1fecd9 commit 4ef74536940ea4c8c7f8c2cb0252bfe5f0db6fdf @josevalim josevalim committed
Showing with 3 additions and 2 deletions.
  1. +3 −2 actionpack/lib/action_dispatch/middleware/request_id.rb
View
5 actionpack/lib/action_dispatch/middleware/request_id.rb
@@ -1,5 +1,6 @@
require 'securerandom'
require 'active_support/core_ext/string/access'
+require 'active_support/core_ext/object/blank'
module ActionDispatch
# Makes a unique request id available to the action_dispatch.request_id env variable (which is then accessible through
@@ -26,8 +27,8 @@ def call(env)
private
def external_request_id(env)
- if env["HTTP_X_REQUEST_ID"].present?
- env["HTTP_X_REQUEST_ID"].gsub(/[^\w\d\-]/, "").first(255)
+ if request_id = env["HTTP_X_REQUEST_ID"].presence
+ request_id.gsub(/[^\w\d\-]/, "").first(255)
end
end

6 comments on commit 4ef7453

@mkdynamic

I'm not convinced that this is actually an improvement:

  1. There is a still a dependency of name in the condition and inside the conditional, except now it is 'request_id'
  2. Adds an assignment in the if condition, which is arguably a risky idiom
  3. It introduces another require (and another dependency)

One alternative to address the 2nd point, although this is possibly harder to parse:

id = env['HTTP_X_REQUEST_ID'].presence and id.gsub(/[^\w\d\-]/, "").first(255)

Very minor, and mainly thinking out loud...

@josevalim
Ruby on Rails member
2) Adds an assignment in the if condition, which is arguably a risky idiom

Deny. It is not a risky idiom in Ruby and it is used over and over in Rails.

3) It introduces another require (and another dependency)

The require had to be there before this commit, but DHH forgot to add it

id = env['HTTP_X_REQUEST_ID'].presence and id.gsub(/[^\w\d\-]/, "").first(255)

If you read the Rails coding guidelines, we explicitly ask to not use and.

@mkdynamic

Appreciate your response, and following Rails' conventions totally makes sense.

Re: inline conditionals, I personally don't have a strong feeling, but I understand the argument that it is a style which is slightly more vulnerable to accidental assignment vs. equality errors. Anyway, it's a waste of time debating that imo.

I don't see any specific reference to not using and, although I am very likely looking in the wrong place:
http://guides.rubyonrails.org/contributing_to_ruby_on_rails.html#follow-the-coding-conventions

Avoiding and makes sense in any case. The precedence difference is too subtle and potentially confusing. I don't think it reads clearly most of the time either.

@vijaydev
Ruby on Rails member

Rgd the reference to not using and, you are looking in the right place. See point 4 in the bulleted list.

@mkdynamic

Well, it says to prefer && / || vs. never use and / or.

In the example above, I think this would be an exception where and is the better choice. It's probably one of the few times it makes sense :)

@mkdynamic

Also, this thread is mostly a waste of time. Sorry, please continue doing useful stuff :)

Please sign in to comment.
Something went wrong with that request. Please try again.