Refactoring the redirect method for the router api.

2 files changed, 24 insertions, 25 deletions

diff --git a/actionpack/lib/action_dispatch/routing/redirection.rb b/actionpack/lib/action_dispatch/routing/redirection.rb
index 35dabae1f2..b7df456e91 100644
--- a/actionpack/lib/action_dispatch/routing/redirection.rb
+++ b/actionpack/lib/action_dispatch/routing/redirection.rb
@@ -3,7 +3,7 @@ require 'active_support/deprecation/reporting'
 
 module ActionDispatch
   module Routing
-    class Redirect
+    class Redirect # :nodoc:
       attr_reader :status, :block
 
       def initialize(status, block)
@@ -35,7 +35,7 @@ module ActionDispatch
       end
     end
 
-    class OptionRedirect < Redirect
+    class OptionRedirect < Redirect # :nodoc:
       alias :options :block
 
       def path(params, request)
@@ -89,33 +89,27 @@ module ActionDispatch
         options = args.last.is_a?(Hash) ? args.pop : {}
         status  = options.delete(:status) || 301
 
+        return OptionRedirect.new(status, options) if options.any?
+
         path = args.shift
 
-        if path.is_a?(String)
-          block_redirect status, lambda { |params, request|
-            (params.empty? || !path.match(/%\{\w*\}/)) ? path : (path % params)
-          }
-        elsif options.any?
-          OptionRedirect.new(status, options)
-        elsif path.respond_to?(:call)
-          block_redirect status, path
-        elsif block
-          if block.arity < 2
-            msg = "redirect blocks with arity of #{block.arity} are deprecated. Your block must take 2 parameters: the environment, and a request object"
-            ActiveSupport::Deprecation.warn msg
-            block_redirect status, lambda { |params, _| block.call(params) }
-          else
-            block_redirect status, block
-          end
-        else
-          raise ArgumentError, "redirection argument not supported"
-        end
-      end
+        block = lambda { |params, request|
+          (params.empty? || !path.match(/%\{\w*\}/)) ? path : (path % params)
+        } if String === path
+
+        block = path if path.respond_to? :call
 
-      private
-        def block_redirect(status, path_proc)
-          Redirect.new status, path_proc
+        # :FIXME: remove in Rails 4.0
+        if block && block.respond_to?(:arity) && block.arity < 2
+          msg = "redirect blocks with arity of #{block.arity} are deprecated. Your block must take 2 parameters: the environment, and a request object"
+          ActiveSupport::Deprecation.warn msg
+          block = lambda { |params, _| block.call(params) }
         end
+
+        raise ArgumentError, "redirection argument not supported" unless block
+
+        Redirect.new status, block
+      end
     end
   end
 end
diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb
index c887fe7e6a..19eee379fd 100644
--- a/actionpack/test/dispatch/routing_test.rb
+++ b/actionpack/test/dispatch/routing_test.rb
@@ -2299,6 +2299,11 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
     assert_equal "/forced_collision", routes_forced_collision_path
   end
 
+  def test_redirect_argument_error
+    routes = Class.new { include ActionDispatch::Routing::Redirection }.new
+    assert_raises(ArgumentError) { routes.redirect Object.new }
+  end
+
   def test_explicitly_avoiding_the_named_route
     assert !respond_to?(:routes_no_collision_path)
   end