Skip to content
Browse files

Relpace `=~ Regexp.new str` with `.include? str` in AC::Base#_valid_a…

…ction_name?

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.
  • Loading branch information...
1 parent 96f28b6 commit 453cd7b6176dd9cb2acc2a597b096ddadf6790a5 @DNNX DNNX committed Jun 19, 2014
Showing with 1 addition and 1 deletion.
  1. +1 −1 actionpack/lib/abstract_controller/base.rb
View
2 actionpack/lib/abstract_controller/base.rb
@@ -255,7 +255,7 @@ def method_for_action(action_name)
# 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

0 comments on commit 453cd7b

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