From 453cd7b6176dd9cb2acc2a597b096ddadf6790a5 Mon Sep 17 00:00:00 2001 From: Viktar Basharymau Date: Thu, 19 Jun 2014 18:17:42 +0300 Subject: [PATCH] Relpace `=~ Regexp.new str` with `.include? str` in AC::Base#_valid_action_name? MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Because it is more natural way to test substring inclusion. Also, in this particular case it is much faster. In general, using `Regexp.new str` for such kind of things is dangerous. The string must be escaped, unless you know what you're doing. Example: Regexp.new "\\" # HELLO WINDOWS # RegexpError: too short escape sequence: /\/ The right way to do this is escape the string Regexp.new Regexp.escape "\\" # => /\\/ Here is the benchmark showing how faster `include?` call is. ``` require 'benchmark/ips' Benchmark.ips do |x| x.report('include?') { !"index".to_s.include? File::SEPARATOR } x.report(' !~ ') { "index" !~ Regexp.new(File::SEPARATOR) } end __END__ Calculating ------------------------------------- include? 75754 i/100ms !~ 21089 i/100ms ------------------------------------------------- include? 3172882.3 (±4.5%) i/s - 15832586 in 5.000659s !~ 322918.8 (±8.6%) i/s - 1602764 in 4.999509s ``` Extra `.to_s` call is needed to handle the case when `action_name` is `nil`. If it is omitted, some tests fail. --- actionpack/lib/abstract_controller/base.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actionpack/lib/abstract_controller/base.rb b/actionpack/lib/abstract_controller/base.rb index acdfb33efa..15faabf977 100644 --- a/actionpack/lib/abstract_controller/base.rb +++ b/actionpack/lib/abstract_controller/base.rb @@ -255,7 +255,7 @@ module AbstractController # Checks if the action name is valid and returns false otherwise. def _valid_action_name?(action_name) - action_name !~ Regexp.new(File::SEPARATOR) + !action_name.to_s.include? File::SEPARATOR end end end