Context: I'm working on a startup MVP that's received very little love in the way of refactoring. This has been the most complex codebase I've had to grapple with, and its been tons of fun figuring out how to breathe life back into it.
But this next bit is impressive. There are a variety of scopes on a bunch of different models that have accumulated some considerable length over the years. What started out as a quite readable/understandable 5-line query has turned into this in 13 commits over 3 years:
class Deal < ActiveRecord::Base # ... deal code scope :visible_to, ->(user) do if user.nil? table = Deal.arel_table condition = table[:tutorial_type].eq('Investor') where(condition) else table = Deal.arel_table di_table = DealInvitation.arel_table project_table = Project.arel_table inv_table = Investment.arel_table client_table = Clients::Client.arel_table investment_access = Investments::InvestmentAccess.arel_table condition = table[:sponsor_id].eq(user.id) .or(table[:mode].eq(Deal.modes['Public']).and(table[:workflow_state].eq('funded'))) .or(((table[:mode].eq(Deal.modes['Public']).and(table[:workflow_state].in('approved'))) .or(table[:id].in( di_table.project(:deal_id).where(di_table[:invitee_id].eq(user.id).and(di_table[:rejected].eq(false)) ) )) ).and(table[:ends_at].gteq(Date.today))) .or(table[:id].in( inv_table.project(:deal_id).where(inv_table[:user_id].eq(user.id) ) )) .or(table[:project_id].in( inv_table.project(:project_id).where(inv_table[:user_id].eq(user.id)))) .or(table[:id].in(inv_table.project(:deal_id).where(inv_table[:id].in(investment_access.project(:investment_id).where(investment_access[:shared_to_id].eq(user.id)))))) .or(table[:project_id].in(project_table.project(:id).where(project_table[:company_id].in(user.company_users.where(role:CompanyUsers::VIEWER_ROLES).pluck(:company_id))))) .or(table[:tutorial_type].eq('Investor')) .or(table[:mode].in([Deal.modes['Public'], Deal.modes['Private']]) .and(Arel.sql((!user.is_fa_accessor.nil? && user.is_fa_accessor).to_s))) .or(table[:tutorial_type].eq('SponsorExample').and(Arel.sql((user.type == 'Sponsor').to_s))) .or(table[:tutorial_type].eq('SponsorSandbox').and(Arel.sql((user.type == 'Sponsor').to_s))) .or(Arel.sql(user.is_admin?.to_s)) where(condition) end end # ... rest of deal code end
Now the thing is, it's not just this one, but there are a variety of models with scopes like this that are used all over the place in controllers/other business logic. When I come across these in the code, I need to be able to introduce changes, I want to actually be able to understand whats going on. I can't go talk with the previous developers about why they added the code they did, and these commits/tests/other artifacts in the codebase aren't that descriptive.
So - how might I approach breaking these things down?
Current approach to try:
But some deeper questions:
Man, what a gig... first, my condolences to any shred of sanity you once had...
Really I think what you outline is pretty good, you have to understand what it's even doing and how it's used throughout before you can even begin to think about refactoring. Have you generated an ERD for the database yet? https://github.com/voormedia/rails-erd I found this can be really helpful when trying to wrap my head around a new project and finding new ways of working with the data.
Regarding AREL, while I don't have a ton of knowledge/experience here, I've always heard stick to ActiveRecord as best you can, and only drop down to AREL when it's absolutely necessary. However, in your case, I'd think you'll need to learn enough AREL to understand exactly what each of the queries you're working with is doing and possibly reimplement some as they may be too complex for your standard AR helpers. Here's a pretty good write-up on AREL: http://jpospisil.com/2014/06/16/the-definitive-guide-to-arel-the-sql-manager-for-ruby.html
As for conclusions to draw, I suppose everyone will have their own to draw here based on their experience, but I wouldn't waste much time thinking about it unless you plan on taking action based on the conclusion. Otherwise, I'd just throw on some tunes and get to crack'n on the refactor :)
That ERD thing has been incredibly useful, though not in this specific instance. This database is crazy.
ActiveRecord uses AREL, and all it really is under the hood is an abstract syntax tree that builds up queries in a db-agnostic way before they can be translated into the actual, right sql for the given db. I'm also guessing that the main reason this was adopted was becuase AR didn't have support for
.or back when it was written, so I think a lot of this could be moved out into AR-scopes now. The reason this got so big is because instead of using controller concerns which combine various scopes pertinent to the situation, they just directly called this on the model that accreted ALL the scopes over time.
There's no pressing business case to refactor this yet, and I think the right timing would be with a deeper update (say moving from api v1 to v2). Then it wouldn't be breaking it apart, necessarily, but just building new/better code around.
Join 24,647+ developers who get early access to new screencasts, articles, guides, updates, and more.