Navigation Menu

Skip to content

Commit

Permalink
Using Object#in? and Object#either? in various places
Browse files Browse the repository at this point in the history
There're a lot of places in Rails source code which make a lot of sense to switching to Object#in? or Object#either? instead of using [].include?.
  • Loading branch information
sikachu authored and dhh committed Apr 10, 2011
1 parent 635d991 commit a9f3c9d
Show file tree
Hide file tree
Showing 44 changed files with 108 additions and 52 deletions.
5 changes: 3 additions & 2 deletions actionpack/lib/action_controller/metal/mime_responds.rb
@@ -1,5 +1,6 @@
require 'abstract_controller/collector'
require 'active_support/core_ext/class/attribute'
require 'active_support/core_ext/object/inclusion'

module ActionController #:nodoc:
module MimeResponds #:nodoc:
Expand Down Expand Up @@ -248,9 +249,9 @@ def collect_mimes_from_class_level #:nodoc:
config = self.class.mimes_for_respond_to[mime]

if config[:except]
!config[:except].include?(action)
!action.in?(config[:except])
elsif config[:only]
config[:only].include?(action)
action.in?(config[:only])
else
true
end
Expand Down
5 changes: 3 additions & 2 deletions actionpack/lib/action_dispatch/routing/mapper.rb
@@ -1,6 +1,7 @@
require 'erb'
require 'active_support/core_ext/hash/except'
require 'active_support/core_ext/object/blank'
require 'active_support/core_ext/object/inclusion'
require 'active_support/inflector'
require 'action_dispatch/routing/redirection'

Expand Down Expand Up @@ -1345,11 +1346,11 @@ def scope_action_options #:nodoc:
end

def resource_scope? #:nodoc:
[:resource, :resources].include?(@scope[:scope_level])
@scope[:scope_level].either?(:resource, :resources)
end

def resource_method_scope? #:nodoc:
[:collection, :member, :new].include?(@scope[:scope_level])
@scope[:scope_level].either?(:collection, :member, :new)
end

def with_exclusive_scope
Expand Down
@@ -1,3 +1,5 @@
require 'active_support/core_ext/object/inclusion'

module ActionDispatch
module Assertions
# A small suite of assertions that test responses from \Rails applications.
Expand Down Expand Up @@ -33,7 +35,7 @@ module ResponseAssertions
def assert_response(type, message = nil)
validate_request!

if [ :success, :missing, :redirect, :error ].include?(type) && @response.send("#{type}?")
if type.either?(:success, :missing, :redirect, :error) && @response.send("#{type}?")
assert_block("") { true } # to count the assertion
elsif type.is_a?(Fixnum) && @response.response_code == type
assert_block("") { true } # to count the assertion
Expand Down
@@ -1,4 +1,5 @@
require 'action_controller/vendor/html-scanner'
require 'active_support/core_ext/object/inclusion'

#--
# Copyright (c) 2006 Assaf Arkin (http://labnotes.org)
Expand Down Expand Up @@ -441,7 +442,7 @@ def assert_select_rjs(*args, &block)

if matches
assert_block("") { true } # to count the assertion
if block_given? && !([:remove, :show, :hide, :toggle].include? rjs_type)
if block_given? && !rjs_type.either?(:remove, :show, :hide, :toggle)
begin
@selected ||= nil
in_scope, @selected = @selected, matches
Expand Down
3 changes: 2 additions & 1 deletion actionpack/lib/action_dispatch/testing/integration.rb
@@ -1,6 +1,7 @@
require 'stringio'
require 'uri'
require 'active_support/core_ext/kernel/singleton_class'
require 'active_support/core_ext/object/inclusion'
require 'active_support/core_ext/object/try'
require 'rack/test'
require 'test/unit/assertions'
Expand Down Expand Up @@ -320,7 +321,7 @@ def reset!
define_method(method) do |*args|
reset! unless integration_session
# reset the html_document variable, but only for new get/post calls
@html_document = nil unless %w(cookies assigns).include?(method)
@html_document = nil unless method.either?("cookies", "assigns")
integration_session.__send__(method, *args).tap do
copy_session_variables!
end
Expand Down
3 changes: 2 additions & 1 deletion actionpack/test/controller/mime_responds_test.rb
@@ -1,6 +1,7 @@
require 'abstract_unit'
require 'controller/fake_models'
require 'active_support/core_ext/hash/conversions'
require 'active_support/core_ext/object/inclusion'

class StarStarMimeController < ActionController::Base
layout nil
Expand Down Expand Up @@ -158,7 +159,7 @@ def rescue_action(e)

protected
def set_layout
if ["all_types_with_layout", "iphone_with_html_response_type"].include?(action_name)
if action_name.either?("all_types_with_layout", "iphone_with_html_response_type")
"respond_to/layouts/standard"
elsif action_name == "iphone_with_html_response_type_without_layout"
"respond_to/layouts/missing"
Expand Down
7 changes: 4 additions & 3 deletions actionpack/test/controller/resources_test.rb
@@ -1,6 +1,7 @@
require 'abstract_unit'
require 'active_support/core_ext/object/try'
require 'active_support/core_ext/object/with_options'
require 'active_support/core_ext/object/inclusion'

class ResourcesController < ActionController::Base
def index() render :nothing => true end
Expand Down Expand Up @@ -1292,7 +1293,7 @@ def assert_named_route(expected, route, options)
def assert_resource_methods(expected, resource, action_method, method)
assert_equal expected.length, resource.send("#{action_method}_methods")[method].size, "#{resource.send("#{action_method}_methods")[method].inspect}"
expected.each do |action|
assert resource.send("#{action_method}_methods")[method].include?(action),
assert action.in?(resource.send("#{action_method}_methods")[method])
"#{method} not in #{action_method} methods: #{resource.send("#{action_method}_methods")[method].inspect}"
end
end
Expand Down Expand Up @@ -1329,9 +1330,9 @@ def assert_whether_allowed(allowed, not_allowed, options, action, path, method)
options = options.merge(:action => action.to_s)
path_options = { :path => path, :method => method }

if Array(allowed).include?(action)
if action.in?(Array(allowed))
assert_recognizes options, path_options
elsif Array(not_allowed).include?(action)
elsif action.in?(Array(not_allowed))
assert_not_recognizes options, path_options
end
end
Expand Down
3 changes: 2 additions & 1 deletion actionpack/test/dispatch/routing_test.rb
@@ -1,6 +1,7 @@
require 'erb'
require 'abstract_unit'
require 'controller/fake_controllers'
require 'active_support/core_ext/object/inclusion'

class TestRoutingMapper < ActionDispatch::IntegrationTest
SprocketsApp = lambda { |env|
Expand Down Expand Up @@ -495,7 +496,7 @@ def self.call(params, request)
resources :todos, :id => /\d+/
end

scope '/countries/:country', :constraints => lambda { |params, req| %[all France].include?(params[:country]) } do
scope '/countries/:country', :constraints => lambda { |params, req| params[:country].either?("all", "france") } do
match '/', :to => 'countries#index'
match '/cities', :to => 'countries#cities'
end
Expand Down
3 changes: 2 additions & 1 deletion actionpack/test/template/erb_util_test.rb
@@ -1,4 +1,5 @@
require 'abstract_unit'
require 'active_support/core_ext/object/inclusion'

class ErbUtilTest < Test::Unit::TestCase
include ERB::Util
Expand Down Expand Up @@ -29,7 +30,7 @@ def test_html_escape_passes_html_escpe_unmodified

def test_rest_in_ascii
(0..127).to_a.map {|int| int.chr }.each do |chr|
next if %w(& " < >).include?(chr)
next if chr.in?('&"<>')
assert_equal chr, html_escape(chr)
end
end
Expand Down
3 changes: 2 additions & 1 deletion actionpack/test/template/form_helper_test.rb
@@ -1,5 +1,6 @@
require 'abstract_unit'
require 'controller/fake_models'
require 'active_support/core_ext/object/inclusion'

class FormHelperTest < ActionView::TestCase
tests ActionView::Helpers::FormHelper
Expand Down Expand Up @@ -1743,7 +1744,7 @@ def test_form_for_with_labelled_builder
def snowman(method = nil)
txt = %{<div style="margin:0;padding:0;display:inline">}
txt << %{<input name="utf8" type="hidden" value="&#x2713;" />}
if (method && !['get','post'].include?(method.to_s))
if method && !method.to_s.either?('get', 'post')
txt << %{<input name="_method" type="hidden" value="#{method}" />}
end
txt << %{</div>}
Expand Down
3 changes: 2 additions & 1 deletion actionpack/test/template/form_options_helper_test.rb
@@ -1,5 +1,6 @@
require 'abstract_unit'
require 'tzinfo'
require 'active_support/core_ext/object/inclusion'

class Map < Hash
def category
Expand Down Expand Up @@ -82,7 +83,7 @@ def test_collection_options_with_preselected_and_disabled_value
def test_collection_options_with_proc_for_disabled
assert_dom_equal(
"<option value=\"&lt;Abe&gt;\">&lt;Abe&gt; went home</option>\n<option value=\"Babe\" disabled=\"disabled\">Babe went home</option>\n<option value=\"Cabe\" disabled=\"disabled\">Cabe went home</option>",
options_from_collection_for_select(dummy_posts, "author_name", "title", :disabled => lambda{|p| %w(Babe Cabe).include? p.author_name })
options_from_collection_for_select(dummy_posts, "author_name", "title", :disabled => lambda{|p| p.author_name.either?("Babe", "Cabe") })
)
end

Expand Down
3 changes: 2 additions & 1 deletion actionpack/test/template/form_tag_helper_test.rb
@@ -1,4 +1,5 @@
require 'abstract_unit'
require 'active_support/core_ext/object/inclusion'

class FormTagHelperTest < ActionView::TestCase
tests ActionView::Helpers::FormTagHelper
Expand All @@ -13,7 +14,7 @@ def snowman(options = {})

txt = %{<div style="margin:0;padding:0;display:inline">}
txt << %{<input name="utf8" type="hidden" value="&#x2713;" />}
if (method && !['get','post'].include?(method.to_s))
if method && !method.to_s.either?('get','post')
txt << %{<input name="_method" type="hidden" value="#{method}" />}
end
txt << %{</div>}
Expand Down
3 changes: 2 additions & 1 deletion activemodel/lib/active_model/validator.rb
@@ -1,6 +1,7 @@
require 'active_support/core_ext/array/wrap'
require "active_support/core_ext/module/anonymous"
require 'active_support/core_ext/object/blank'
require 'active_support/core_ext/object/inclusion'

module ActiveModel #:nodoc:

Expand Down Expand Up @@ -67,7 +68,7 @@ module ActiveModel #:nodoc:
#
# class TitleValidator < ActiveModel::EachValidator
# def validate_each(record, attribute, value)
# record.errors[attribute] << 'must be Mr. Mrs. or Dr.' unless ['Mr.', 'Mrs.', 'Dr.'].include?(value)
# record.errors[attribute] << 'must be Mr. Mrs. or Dr.' unless value.either?('Mr.', 'Mrs.', 'Dr.')
# end
# end
#
Expand Down
@@ -1,5 +1,6 @@
require "cases/helper"
require 'logger'
require 'active_support/core_ext/object/inclusion'

class SanitizerTest < ActiveModel::TestCase

Expand All @@ -9,7 +10,7 @@ class SanitizingAuthorizer
attr_accessor :logger

def deny?(key)
[ 'admin' ].include?(key)
key.in?(['admin'])
end

end
Expand Down
3 changes: 2 additions & 1 deletion activerecord/lib/active_record/associations/association.rb
@@ -1,4 +1,5 @@
require 'active_support/core_ext/array/wrap'
require 'active_support/core_ext/object/inclusion'

module ActiveRecord
module Associations
Expand Down Expand Up @@ -163,7 +164,7 @@ def interpolate(sql, record = nil)
def creation_attributes
attributes = {}

if [:has_one, :has_many].include?(reflection.macro) && !options[:through]
if reflection.macro.either?(:has_one, :has_many) && !options[:through]
attributes[reflection.foreign_key] = owner[reflection.active_record_primary_key]

if reflection.options[:as]
Expand Down
@@ -1,3 +1,5 @@
require 'active_support/core_ext/object/inclusion'

module ActiveRecord::Associations::Builder
class BelongsTo < SingularAssociation #:nodoc:
self.macro = :belongs_to
Expand Down Expand Up @@ -65,7 +67,7 @@ def add_touch_callbacks(reflection)

def configure_dependency
if options[:dependent]
unless [:destroy, :delete].include?(options[:dependent])
unless options[:dependent].either?(:destroy, :delete)
raise ArgumentError, "The :dependent option expects either :destroy or :delete (#{options[:dependent].inspect})"
end

Expand Down
@@ -1,3 +1,5 @@
require 'active_support/core_ext/object/inclusion'

module ActiveRecord::Associations::Builder
class HasMany < CollectionAssociation #:nodoc:
self.macro = :has_many
Expand All @@ -14,7 +16,7 @@ def build

def configure_dependency
if options[:dependent]
unless [:destroy, :delete_all, :nullify, :restrict].include?(options[:dependent])
unless options[:dependent].either?(:destroy, :delete_all, :nullify, :restrict)
raise ArgumentError, "The :dependent option expects either :destroy, :delete_all, " \
":nullify or :restrict (#{options[:dependent].inspect})"
end
Expand Down
@@ -1,3 +1,5 @@
require 'active_support/core_ext/object/inclusion'

module ActiveRecord::Associations::Builder
class HasOne < SingularAssociation #:nodoc:
self.macro = :has_one
Expand Down Expand Up @@ -27,7 +29,7 @@ def validate_options

def configure_dependency
if options[:dependent]
unless [:destroy, :delete, :nullify, :restrict].include?(options[:dependent])
unless options[:dependent].either?(:destroy, :delete, :nullify, :restrict)
raise ArgumentError, "The :dependent option expects either :destroy, :delete, " \
":nullify or :restrict (#{options[:dependent].inspect})"
end
Expand Down
@@ -1,3 +1,5 @@
require 'active_support/core_ext/object/inclusion'

module ActiveRecord
# = Active Record Belongs To Has One Association
module Associations
Expand Down Expand Up @@ -50,7 +52,7 @@ def set_new_record(record)
end

def remove_target!(method)
if [:delete, :destroy].include?(method)
if method.either?(:delete, :destroy)
target.send(method)
else
nullify_owner_attributes(target)
Expand Down
@@ -1,4 +1,5 @@
require 'active_support/core_ext/class/attribute'
require 'active_support/core_ext/object/inclusion'

module ActiveRecord
module AttributeMethods
Expand Down Expand Up @@ -58,7 +59,7 @@ def #{attr_name}=(original_time)

private
def create_time_zone_conversion_attribute?(name, column)
time_zone_aware_attributes && !self.skip_time_zone_conversion_for_attributes.include?(name.to_sym) && [:datetime, :timestamp].include?(column.type)
time_zone_aware_attributes && !self.skip_time_zone_conversion_for_attributes.include?(name.to_sym) && column.type.either?(:datetime, :timestamp)
end
end
end
Expand Down
4 changes: 3 additions & 1 deletion activerecord/lib/active_record/railties/databases.rake
@@ -1,3 +1,5 @@
require 'active_support/core_ext/object/inclusion'

db_namespace = namespace :db do
task :load_config => :rails_env do
require 'active_record'
Expand Down Expand Up @@ -135,7 +137,7 @@ db_namespace = namespace :db do
end

def local_database?(config, &block)
if %w( 127.0.0.1 localhost ).include?(config['host']) || config['host'].blank?
if config['host'].either?("127.0.0.1", "localhost") || config['host'].blank?
yield
else
$stderr.puts "This task only modifies local databases. #{config['database']} is on a remote host."
Expand Down
3 changes: 2 additions & 1 deletion activerecord/lib/active_record/reflection.rb
@@ -1,5 +1,6 @@
require 'active_support/core_ext/class/attribute'
require 'active_support/core_ext/module/deprecation'
require 'active_support/core_ext/object/inclusion'

module ActiveRecord
# = Active Record Reflection
Expand Down Expand Up @@ -163,7 +164,7 @@ def klass

def initialize(macro, name, options, active_record)
super
@collection = [:has_many, :has_and_belongs_to_many].include?(macro)
@collection = macro.either?(:has_many, :has_and_belongs_to_many)
end

# Returns a new, unsaved instance of the associated class. +options+ will
Expand Down
@@ -1,4 +1,5 @@
require 'rails/generators/active_record'
require 'active_support/core_ext/object/inclusion'

module ActiveRecord
module Generators
Expand All @@ -13,7 +14,7 @@ def create_migration_file

def session_table_name
current_table_name = ActiveRecord::SessionStore::Session.table_name
if ["sessions", "session"].include?(current_table_name)
if current_table_name.either?("sessions", "session")
current_table_name = (ActiveRecord::Base.pluralize_table_names ? 'session'.pluralize : 'session')
end
current_table_name
Expand Down

14 comments on commit a9f3c9d

@tenderlove
Copy link
Member

Choose a reason for hiding this comment

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

This commit breaks actionpack tests on 1.9.2. Please fix. ❤️

I'm happy about adding convenience methods, but is it really necessary to make sweeping changes to rails internals like this?

@dhh
Copy link
Member

@dhh dhh commented on a9f3c9d Apr 10, 2011

Choose a reason for hiding this comment

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

Rails should use its own best practices, but yes, we should make a habit of running the suite under both 1.8.7 and 1.9.2 now to catch any bugs.

@tenderlove
Copy link
Member

Choose a reason for hiding this comment

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

@dhh I don't think we've properly done the du-diligence required to award Object#in? and Object#either? the title of "best practice".

Here's why:

  1. We haven't used this IRL, so it's difficult to say it's actually a best practice.
  2. It's possible for arrays to be cached and duped out of the AST. Removing inline arrays kills that benefit
  3. Every place where we were doing one method call, now we're doing 2
  4. either? uses varargs, which costs us an extra array allocation

Many of the changes made in this commit probably weren't in performance hotspots, but I don't think anyone has done research on that. Especially the changes in actionpack worry me.

@dhh
Copy link
Member

@dhh dhh commented on a9f3c9d Apr 10, 2011

Choose a reason for hiding this comment

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

1: This point is self-evident to me by comparing before/after code using #in? vs #include?
2: This doesn't change that. People are already doing inline arrays to use with include? -- see the code changed in this example.
3: I'm willing to make this sacrifice for the added expressiveness.
4: See 3.

Re: performance, I'd rather we take the general principle that we'll write the best, most beautiful code we can first and then back out of it when it's proven to be a problem.

@phene
Copy link

@phene phene commented on a9f3c9d Apr 10, 2011

Choose a reason for hiding this comment

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

+1

@tenderlove
Copy link
Member

Choose a reason for hiding this comment

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

re #2, the code changed in this example is my point. By switching to either? we've removed the tLBRACK nodes from the AST:

no more brackets

Now there is no possibility to cache the arrays in the AST.

I agree we should write the best looking code and deal with perf stuff later. I just can't help from commenting because the perf degradations are so apparent to me and the supposed readability win seem minimal in comparison. :-(

Anyway, once the tests pass, I'll stop complaining. ;-)

@dhh
Copy link
Member

@dhh dhh commented on a9f3c9d Apr 10, 2011

Choose a reason for hiding this comment

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

Can you come up with a case that shows the performance degradation (or maybe someone else can?)? I'm all for beauty, but sure, if there's a noticeable (even if smallish) ding to performance, it's not worth it at the framework level. Also, we can switch that one to .in?([ :references, :belongs_to ]) and we're still ahead. But let's first see some data showing this to be a problem.

@tenderlove
Copy link
Member

Choose a reason for hiding this comment

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

Just calling 2 methods vs calling 1 method is a perf ding (on mri). I didn't think we'd need benchmarks to prove that.

Only frequent code paths are important. But like I said, I don't think these changes are in hotspots. I am just afraid to add this many changes at once!

@evanphx
Copy link
Contributor

Choose a reason for hiding this comment

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

While it's certainly up to you, I'd suggest not using this idiom if there are only 2 values being tested for. The performance difference between just doing the 2 comparisons in the if and calling out to #either? is radically different. Additionally, it can be just as easy to read and write.

@evanphx
Copy link
Contributor

Choose a reason for hiding this comment

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

@dhh Here is a benchmark for ya: https://gist.github.com/912835

@sikachu
Copy link
Member Author

Choose a reason for hiding this comment

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

@tenderlove I'm fixing the test case now. I was running the isolated test locally myself and saw that error too, but I thought somebody broke it first because I never be able to run the whole green test suite. Shame on me.

The reason I propose the change internal core was 1) Someone said that if I'm going to add a method to core_ext I should be able to show that Rails would be benefits by it too. And 2) Would be the fact that you can see a lot of use cases in those files.

From @evanphx benchmark, That's pretty massive. Is there any way I could improve the method?

@sikachu
Copy link
Member Author

Choose a reason for hiding this comment

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

After further discussion with the core team, I decide to back off a bit and I'm going to change back to using [].include? in a lot of places. This seems like a tradeoff between performance and readable code.

@jamesarosen
Copy link
Contributor

Choose a reason for hiding this comment

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

From what I can tell, the uses of in? in Rails itself are mostly limited to a small operand. Thus, I was hoping that "unrolling" it a bit to would help performance on small inputs. Sadly, it's actually worse on Ruby 1.9.2.

@dhh
Copy link
Member

@dhh dhh commented on a9f3c9d Apr 11, 2011

Choose a reason for hiding this comment

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

This looks like hardly a difference at all at 50% unless it's sitting smack in the middle of a hotspot code path where it's the main time consumer. The vast majority of these changes are in tests or completely non-critical areas. I'd do some quick benches on 3 points: The callback change, the mime respond, and the mapper. Or you could remove those three in a round of premature optimization and leave the other 40 changes in place.

Please sign in to comment.