Skip to content

Commit

Permalink
refactorin'
Browse files Browse the repository at this point in the history
  • Loading branch information
@ajb
ajb committed Jun 3, 2015
1 parent 0b962af commit e29d20b
Show file tree
Hide file tree
Showing 3 changed files with 57 additions and 55 deletions.
2 changes: 1 addition & 1 deletion README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ class PersonFilterer < Filterer::Base
# '?sort=data1', '?sort=data2', etc. will call the following proc, passing the
# query and match data
sort_option Regexp.new('data([0-9]+)'), -> (query, matches, filterer) {
query.order "(ratings -> '#{matches[1]}') #{filterer.direction}"
query.order "(ratings -> '#{matches[1]}') #{filterer.sort_direction}"
}
end
```
Expand Down
92 changes: 49 additions & 43 deletions lib/filterer/base.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ module Filterer
class Base
attr_accessor :results,
:meta,
:direction,
:sort,
:params,
:opts
Expand All @@ -21,14 +20,14 @@ class Base

class << self
# Macro for adding sort options
def sort_option(key, query_string_or_proc = nil, opts = {})
if query_string_or_proc.is_a?(Hash)
opts, query_string_or_proc = query_string_or_proc.clone, nil
def sort_option(key, string_or_proc = nil, opts = {})
if string_or_proc.is_a?(Hash)
opts, string_or_proc = string_or_proc.clone, nil
end

if !query_string_or_proc
if !string_or_proc
if key.is_a?(String)
query_string_or_proc = key
string_or_proc = key
else
raise 'Please provide a query string or a proc.'
end
Expand All @@ -38,13 +37,13 @@ def sort_option(key, query_string_or_proc = nil, opts = {})
raise "Default sort option can't have a Regexp key."
end

if query_string_or_proc.is_a?(Proc) && opts[:tiebreaker]
if string_or_proc.is_a?(Proc) && opts[:tiebreaker]
raise "Tiebreaker can't be a proc."
end

self.sort_options += [{
key: key,
query_string_or_proc: query_string_or_proc,
string_or_proc: string_or_proc,
opts: opts
}]
end
Expand Down Expand Up @@ -76,6 +75,10 @@ def starting_query
raise 'You must override this method!'
end

def sort_direction
params[:direction].try(:downcase) == 'desc' ? 'desc' : 'asc'
end

private

def paginate_results
Expand Down Expand Up @@ -117,33 +120,13 @@ def present_params
end

def order_results
self.direction = params[:direction].try(:downcase) == 'desc' ? 'desc' : 'asc'
self.sort = if (params[:sort] && get_sort_option(params[:sort]))
params[:sort]
else
default_sort_param
end

if !get_sort_option(sort)
self.results = results.order default_sort_sql
elsif get_sort_option(sort)[:query_string_or_proc].is_a?(String)
self.results = results.order basic_sort_sql
elsif get_sort_option(sort)[:query_string_or_proc].is_a?(Proc)
apply_sort_proc
end
end

def default_sort_sql
"#{results.model.table_name}.id asc"
end

def basic_sort_sql
%{
#{get_sort_option(sort)[:query_string_or_proc]}
#{direction}
#{get_sort_option(sort)[:opts][:nulls_last] ? 'NULLS LAST' : ''}
#{tiebreaker_sort_string ? ',' + tiebreaker_sort_string : ''}
}.squish
self.results = if !sort_option
results.order(default_sort_sql)
elsif sort_option[:string_or_proc].is_a?(String)
results.order(basic_sort_sql)
elsif sort_option[:string_or_proc].is_a?(Proc)
apply_sort_proc
end
end

def per_page
Expand All @@ -158,13 +141,17 @@ def current_page
[params[:page].to_i, 1].max
end

def apply_sort_proc
sort_key = get_sort_option(sort)[:key]
matches = sort_key.is_a?(Regexp) && sort.match(sort_key)
self.results = get_sort_option(sort)[:query_string_or_proc].call(results, matches, self)
def sort_option
@sort_option ||= begin
if params[:sort] && (opt = find_sort_option_from_param(params[:sort]))
opt
else
default_sort_option
end
end
end

def get_sort_option(x)
def find_sort_option_from_param(x)
self.class.sort_options.detect do |sort_option|
if sort_option[:key].is_a?(Regexp)
x.match(sort_option[:key])
Expand All @@ -174,16 +161,35 @@ def get_sort_option(x)
end
end

def default_sort_param
def default_sort_sql
"#{results.model.table_name}.id asc"
end

def basic_sort_sql
%{
#{sort_option[:string_or_proc]}
#{sort_direction}
#{sort_option[:opts][:nulls_last] ? 'NULLS LAST' : ''}
#{tiebreaker_sort_string ? ', ' + tiebreaker_sort_string : ''}
}.squish
end

def apply_sort_proc
sort_key = sort_option[:key]
matches = sort_key.is_a?(Regexp) && params[:sort].match(sort_key)
sort_option[:string_or_proc].call(results, matches, self)
end

def default_sort_option
self.class.sort_options.detect do |sort_option|
sort_option[:opts][:default]
end.try(:[], :key)
end
end

def tiebreaker_sort_string
self.class.sort_options.detect do |sort_option|
sort_option[:opts][:tiebreaker]
end.try(:[], :query_string_or_proc)
end.try(:[], :string_or_proc)
end
end
end
18 changes: 7 additions & 11 deletions spec/lib/filterer/base_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ def starting_query
end

sort_option 'id', default: true
sort_option Regexp.new('foo([0-9]+)'), -> (results, matches, filterer) { results.order(matches[1] + ' ' + filterer.direction) }
sort_option Regexp.new('foo([0-9]+)'), -> (results, matches, filterer) { results.order(matches[1] + ' ' + filterer.sort_direction) }
end

class SortingFiltererD < Filterer::Base
Expand All @@ -96,7 +96,7 @@ def starting_query
end

sort_option 'id', default: true
sort_option Regexp.new('foo([0-9]+)'), -> (results, matches, filterer) { results.order(matches[1] + ' ' + filterer.direction) }
sort_option Regexp.new('foo([0-9]+)'), -> (results, matches, filterer) { results.order(matches[1] + ' ' + filterer.sort_direction) }
sort_option Regexp.new('zoo([0-9]+)'), -> (results, matches, filterer) { results.order('zoo') }
end

Expand Down Expand Up @@ -166,35 +166,33 @@ class PaginationFiltererInherit < PaginationFiltererB
end

it 'applies a default sort' do
expect_any_instance_of(FakeQuery).to receive(:order).with('id asc').and_return(FakeQuery.new)
filterer = SortingFiltererA.new
expect(filterer.sort).to eq('id')
end

it 'applies a default sort when inheriting a class' do
expect_any_instance_of(FakeQuery).to receive(:order).with('id asc').and_return(FakeQuery.new)
filterer = InheritedSortingFiltererA.new
expect(filterer.sort).to eq('id')
end

it 'can include a tiebreaker' do
expect_any_instance_of(FakeQuery).to receive(:order).with(/thetiebreaker/).and_return(FakeQuery.new)
expect_any_instance_of(FakeQuery).to receive(:order).with('id asc , thetiebreaker').and_return(FakeQuery.new)
filterer = SortingFiltererB.new
expect(filterer.sort).to eq('id')
end

it 'can match by regexp' do
expect_any_instance_of(FakeQuery).to receive(:order).with('111 asc').and_return(FakeQuery.new)
filterer = SortingFiltererC.new(sort: 'foo111')
expect(filterer.sort).to eq('foo111')
end

it 'does not choke on a nil param' do
expect_any_instance_of(FakeQuery).to receive(:order).with('id asc').and_return(FakeQuery.new)
filterer = SortingFiltererC.new
expect(filterer.sort).to eq('id')
end

it 'can apply a proc' do
expect_any_instance_of(FakeQuery).to receive(:order).with('111 asc').and_return(FakeQuery.new)
filterer = SortingFiltererC.new(sort: 'foo111')
expect(filterer.sort).to eq('foo111')
end

it 'can put nulls last' do
Expand All @@ -210,13 +208,11 @@ class PaginationFiltererInherit < PaginationFiltererB
it 'can distinguish between two regexps' do
expect_any_instance_of(FakeQuery).to receive(:order).with('zoo').and_return(FakeQuery.new)
filterer = SortingFiltererE.new(sort: 'zoo111')
expect(filterer.sort).to eq('zoo111')
end

it 'can distinguish between two regexps part 2' do
expect_any_instance_of(FakeQuery).to receive(:order).with('111 asc').and_return(FakeQuery.new)
filterer = SortingFiltererE.new(sort: 'foo111')
expect(filterer.sort).to eq('foo111')
end

it 'throws an error when key is a regexp and no query string given' do
Expand Down

0 comments on commit e29d20b

Please sign in to comment.