Questions about Fat-Model, Skinny-Controller practice

Question

How do you decide that a line of code must be moved from controller to model?

I've done some research already about this best practice and here's the best explanations so far, but I hope you guys can expand this well because my newbie brain can't fully comprehend it :)

1. any non-response-related logic should go in the model
2. you only need process request parameters and initialize model layer in controllers
3. Business logic need to be implemented in model layer

EDIT: Let's say I want to refactor my controller below from:

class TaskController < ApplicationController
  def index
    @tasks = Task.find_all_by_complete(:false, :order => "created_at DESC")
  end
end

to

class TaskController < ApplicationController
  def index
    @tasks = Task.find_incomplete
  end
end

Which of these 2 code blocks are correct?

class Task < ActiveRecord::Base
  def self.find_incomplete
    find_all_by_complete(:false, :order => "created_at DESC")
  end
end

or

class Task < ActiveRecord::Base
  def find_incomplete
    self.find_all_by_complete(:false, :order => "created_at DESC")
  end
end

EDIT2: If I want to refactor my controller below from:

@average_review = @surf_school.surf_school_reviews.average(:rating).round(2)

to

@average_review = @surf_school.average_review

my code inside model should be:

def average_review
  self.surf_school_reviews.average(:rating).round(2)
end

Answer 1

Generally as a starting point I would place any CRUD related code in the Model. Setting up custom methods in your models so you can call @user.invalidate or @post.replace_owner(@user). These instances of @user can be referenced in the model as self. Example:

class Post < ApplicationRecord
  def add_owner(user)
    self.update_attributes(user_id: user.id)
  end
end

So moving logic like that into the model allows for cleaner controller code. I would also handle requests, setting up variables, using model methods and determine which page to load in the Controller.

Edit: I would also go with the first code block:

def self.find_incomplete
  find_all_by_complete(:false, :order => "created_at DESC")
end

That is precisely how you create an ActiveRecord scope manually which is identicle to the following:

class Task < ActiveRecord::Base
  scope :find_complete -> { find_all_by_complete(:false, :order => "created_at DESC") }
end

But this is just syntactic sugar as they say. Your first block is exactly what my block above gets transformed into, so to speak.

See more on ActiveRecord::Scoping.

Answer 2

At my work, we have multiple classes for each model in the model layer. To simplify things, we have a DTO and DAO object. DTO's contain fields and DAO's contain methods to save, modify and delete a database entry.

Some of our DAOs also contain helper methods with special/more complex search queries and override standard save methods to prevent the controller from entering invalid data into our models.

Our controller actions fill up the models using the DAOs to a certain point and saves their new state into the database. Our controllers contain the business logic but some DAOs have methods that do a little extra to prevent invalid data from entering out database. It makes the business logic more readable and easier to understand.

Also a thing to keep in mind is, it's perfect if you follow a certain practice but consider going sideways if it doesn't meet your requirements. Like, if you happen to have business logic that is mandatory to be performed before each save action, I'd honestly either define a utility/helper class that has a static method holding this logic or override the save method of your model object and move it over there.

---

Note: DAO stands for Data Access Object. This is common in our use case but you'd have just one class that basically does the same instead of having multiple classes. It was a design pattern that was chosen before I even started working here.

---

Edit: I'd go for the second option. But this post might help you out even more: What does def `self.function` name mean?