Is that a proper way to refactor ActiveRecord fat models?

Question

If for example I've this ActiveRecord model:

app/models/order.rb

class Order < ActiveRecord::Base
  # model logic
end
require "lib/someclass.rb"

lib/somelass.rb

class Order
  before_save :something
  # more logic here
end

Is that a good way to refactor/extract logic from model? Or maybe use concern class, service class or something else?

Answer 1

Like someone told me a long time ago:

Code refactoring is not a matter of randomly moving code around.

In your example that is exactly what you are doing: moving code into another file

Why is it bad?

By moving code around like this, you are making your original class more complicated since the logic is randomly split into several other classes. Of course it looks better, less code in one file is visually better but that's all.

Prefer composition to inheritance. Using mixins like this is asking to "cleaning" a messy room by dumping the clutter into six separate junk drawers and slamming them shut. Sure, it looks cleaner at the surface, but the junk drawers actually make it harder to identify and implement the decompositions and extractions necessary to clarify the domain model.

What should I do then?

You should ask yourself:

• Which code goes together and could be part of a new class / module ?
• Where does it makes sense to extract code to somewhere else ?
• Do I have some piece of code that is shared across my application ?
• Can I extract recurrent patterns in my code base ?

Extract Service Object

I reach for Service Objects when an action meets one or more of these criteria:

• The action is complex
• The action reaches across multiple models
• The action interacts with an external service
• The action is not a core concern of the underlying model
• There are multiple ways of performing the action

Extract Form Objects

When multiple model can be updated by a a single form submission, you might want to create a Form Object.

This enable to put all the form logic (name conventions, validations and so on) into one place.

Extract Query Objects

You should extract complex SQL/NoSQL queries into their own class. Each Query Object is responsible for returning a result set based on the criterias / business rules.

Extract Presenters / Decorators

Extract views logic into presenters. Your model should not deal with specific views logic. Moreover, it will enable you to use your presenter in multiple views.

More on decorators

Thanks to this blog post to help me putting these together.

Answer 2

Keeping the code in the same class moves logic, it doesn't extract it.

Externalizing callback declaration is misleading and potentially dangerous. Callbacks are abused enough; forcing readers to hunt down related files is cruel.

There's no general way to answer the question as asked; the "best" refactors depends on what's actually being refactoried. Lifecycle information should be obvious and precise, though.

Concerns, services, decorators, facades, etc. are good mechanisms that support refactoring. Without knowing what's being refactored it's impossible to provide meaningful advice regarding what's "best".