Recently I was working on refactoring some unwieldy code for filtering data. Here's a short story of how it went 🙂.
The code I was refactoring was large, but one repetition particularly stood out:
# app/models/user.rb def self.filter(filters) if filters[:status].present? User.where(status: filters[:status] end if filters[:location_id].present? User.where(location_id: filters[:location_id]) end # and so on... end
The UI had a large set of filters for filtering through a list - which is what this code does - filters through records from database.
My first thoughts were:
Why is this code so verbose?
Why do we need those
ifconditions? Without them, we can just chain those
whereclauses and this code would read so much better!
Let us try removing the conditions. Ohh wait, this code does not have tests! 😱
Time to flex rspec muscles 💪. Yeah TDD!
# spec/models/user_spec.rb it "filters users by status" do # Create some users - Arrange filtered_users = User.filter(status: "pending").pluck(:id) # Act expect(filtered_users).to match_array(users.map(&:id)) # Assert end
Run tests - passes ✅
Remove "if" - passes ✅ As expected.
Now for the real test.. We naturally expect all records to be returned if no filter is applied.
# spec/models/user_spec.rb it "returns all users if no status filter applied" do # Create some users - Arrange filtered_users = User.filter(status: nil).pluck(:id) # Act expect(filtered_users).to match_array(User.pluck(:id)) # Assert end
Run tests - passes ✅
Remove "if" -
fails ❌ There you go. That is why we need the condition.
To see what exactly is causing this: ** Hops into REPL **
irb(main)> User.count => 10 irb(main)> User.where(status: "pending").count (1.0ms) SELECT COUNT(*) FROM "users" WHERE "users"."status" = $1 [["status", "pending"]] => 4 irb(main)> User.where(status: nil).count (0.9ms) SELECT COUNT(*) FROM "users" WHERE "users"."status" IS NULL => 0
Fair enough. ActiveRecord gets
nil, it filters by
NULL. Let us see if we can find a way to somehow still chain those methods.
ActiveRecord expects a Relation object for chaining to work. Try creating a generic method:
# app/models/user.rb def where_if(filters, attribute:) data = filters[attribute] return all unless data where(attribute => data) end
# app/models/user.rb def self.filter(filters) where_if(filters, :status) .where_if(filters, :location_id) .where_if(filters, :is_active) .where_if(filters, :locale) # and some more... end
Much better! 🎉
Taking it a step further, the application had few more listing pages which means other models with similar
.filter method. Our
.where_if method is generic enough to be used by all of them.
# app/models/concerns/filterable.rb module Filterable def where_if(filters, attribute:, parameter: nil) data = parameter ? filters[parameter] : filters[attribute] return all unless data where(attribute => data) end end
And now we can
include Filterable in all those models having filter. I also went ahead and separated
parameter which helps in cases where parameter name from frontend does not match the database attribute name in backend.