Refactored Article.search into internal methods

lib/nytimes_articles/article.rb

@@ -87,11 +87,7 @@ def self.init_from_api(params)
 
 				article
 			end
-
-			def self.parse_reply(reply)
-				ResultSet.init_from_api(reply)
-			end
-
+
 			def self.search(query, params={})
 				params = params.dup
 
@@ -104,56 +100,85 @@ def self.search(query, params={})
 
 				api_params = {}
 
-				if params[:query]
-					api_params['query'] = params[:query]
-				else
-					query = []
+				add_query_params(api_params, params)
+				add_facets_param(api_params, params)
+				add_rank_params(api_params, params)
+				add_date_params(api_params, params)
+				add_offset_params(api_params, params)
+
+				reply = invoke(api_params)
+				parse_reply(reply)
+			end
+
+private
+			def self.parse_reply(reply)
+				ResultSet.init_from_api(reply)
+			end
+
+			def self.add_facets_param(out_params, in_params)
+				if in_params[:facets]
+					out_params['facets'] = in_params[:facets].to_a
+				end
+			end
+
+			def self.add_query_params(out_params, in_params)
+				query = []
 
-					TEXT_FIELDS.each do |tf|
-						if params[tf.to_sym]
-							query << text_argument(tf, params[tf.to_sym])
-						end
+				query << in_params[:query]
+
+				# Also add other text params to the query
+				TEXT_FIELDS.each do |tf|
+					if in_params[tf.to_sym]
+						query << text_argument(tf, in_params[tf.to_sym])
 					end
-
-					api_params['query'] = query.join(' ')
 				end
-
-				if params[:rank]
-					unless [:newest, :oldest, :closest].include?(params[:rank])
+
+				out_params['query'] = query.compact.join(' ')
+			end
+
+			def self.add_rank_params(out_params, in_params)
+				if in_params[:rank]
+					unless [:newest, :oldest, :closest].include?(in_params[:rank])
 						raise ArgumentError, "Rank should only be :newest | :oldest | :closest"
 					end
 
-					api_params['rank'] = params[:rank].to_s
+					out_params['rank'] = in_params[:rank].to_s
 				end
-
-				if params[:begin_date]
-					api_params['begin_date'] = date_argument(:begin_date, params[:begin_date])
+			end
+
+			def self.add_date_params(out_params, in_params)
+				if in_params[:begin_date]
+					out_params['begin_date'] = date_argument(:begin_date, in_params[:begin_date])
 				end
 
-				if params[:end_date]
-					api_params['end_date'] = date_argument(:end_date, params[:end_date])
+				if in_params[:end_date]
+					out_params['end_date'] = date_argument(:end_date, in_params[:end_date])
 				end
-
-				if params[:page]
-					unless params[:page].is_a? Integer
+			end
+
+			def self.add_offset_params(out_params, in_params)
+				if in_params[:page]
+					unless in_params[:page].is_a? Integer
 						raise ArgumentError, "Page must be an integer"
 					end
 
+					unless in_params[:page] >= 1
+						raise ArgumentError, "Page must count up from 1"
+					end
+
 					# Page counts from 1, offset counts from 0
-					api_params['offset'] = params[:page] - 1
+					out_params['offset'] = in_params[:page] - 1
 				end
 
-				if params[:offset]
-					unless params[:offset].is_a? Integer
+				if in_params[:offset]
+					unless in_params[:offset].is_a? Integer
 						raise ArgumentError, "Offset must be an integer"
 					end
 
-					api_params['offset'] = params[:offset]
+					out_params['offset'] = in_params[:offset]
 				end
-
-				reply = invoke(api_params)
-				parse_reply(reply)
 			end
+
 		end
 	end
 end

test/nytimes/articles/test_article.rb

@@ -18,7 +18,10 @@ def setup
 			Article.search "FOO BAR"
 		end
 
-		should "accept a Hash for the first argument"
+		should "accept a Hash for the first argument" do
+			Article.expects(:invoke).with(has_entry("query", "FOO BAR"))
+			Article.search :query => 'FOO BAR', :page => 2
+		end
 
 		context "date ranges" do
 			should "pass a string argument to begin_date straight through" do
@@ -86,6 +89,10 @@ def setup
 			should "raise an ArgumentError if the page is not an Integer" do
 				assert_raise(ArgumentError) { Article.search :page => 'orange' }
 			end
+
+			should "raise an ArgumentError if the page is less than 1" do
+				assert_raise(ArgumentError) { Article.search :page => 0 }
+			end
 
 			should "use the :offset argument if both an :offset and :page are provided" do
 				Article.expects(:invoke).with(has_entry("offset", 2))