Refactoring Rails Code

I'm an early stage RoR developer who is looking to move away from my heavy controllers/skinny models thinking.
I love building things! That means coding, baking, carpentry, cheesemaking, winemaking, etc... 
I'm a long-time rails dev, and am currently interested in building federated apps/services.
Follow this Taaalk

5 followers

1,252 views

Joshua Summers
13:06, 27 Jul 20
(All code being discussed can be found here: https://github.com/JoshInLisbon/taaalk_edge)
Just some clarity on terms used in the code before we begin:
  • A Tlk is a Taaalk. It belongs to a User
  • A Spkr is a Speaker, which is a participant of a Taaalk. A Spkr belongs to a Tlk and a User.
  • A Msg is a message in a Taaalk. A Msg belongs to a Tlk, a User and a Spkr.
Before we get into looking at my heavy controllers, when it comes to my models, am I overdoing the 'belongs_to' and 'has_many'?
For example, here is my Tlk model:
class Tlk < ApplicationRecord
  belongs_to :user
  has_many :spkrs, dependent: :destroy
  has_many :msgs, dependent: :destroy
  has_many :tlk_follows, dependent: :destroy
  has_many :draft_msgs, dependent: :destroy

  has_one_attached :image

  validates :title, presence: true

  def any_published_msgs?
    msgs.find_by(published: true).present?
  end

  def all_spkrs
    spkrs.sort_by(&:created_at)
  end

  def non_hidden_spkrs
    spkrs.where(hide: false).sort_by(&:created_at)
  end

  def non_tlk_user_spkrs
    spkrs.where.not(user: self.user).sort_by(&:created_at)
  end

  extend FriendlyId
  friendly_id :slug_candidates, use: :slugged

  def slug_candidates
    [
      :title,
      [:title, DateTime.now.to_date]
    ]
  end
end
Is that a bit clumsy as if Msgs belong_to Spkrs, and Spkrs belong_to Tlks, do I need to state the obvious again here? Or is that ok?
And if I were to one day use a list of Tlks to pull messages from, for example if I wanted to show the latest message of each Tlk on the tlk index page, would it matter if Msgs didn't belong_to Tlks directly?
Kent Slaymaker
14:26, 27 Jul 20
Hey there!
First of all, congrats! You already have a functional website that a few people use. That is way further than many personal projects ever get. 
Kent Slaymaker
14:29, 27 Jul 20
You asked if you are too heavy on relationships between models. This is far from the heaviest I've seen. 
That being said, do you have any kind of diagram that shows the relationships? Like a UML or something? 
Joshua Summers
16:05, 27 Jul 20
I just made this:

image.png 60.6 KB

Which explains the current relations.
I guess if it is the case that a Spkr has a Msg, and a Tlk has a Spkr, does it make any difference if a Tlk has a Msg? Or was that just a wasted line of code?
(Controllers after this.)
Jaryl Sim
08:28, 28 Jul 20
Agreed with Kent that this model isn't all that bad, but one improvement would be to move some of that `spkrs` code out of the model. At the very least, you can implement them as named scopes, but for me what I like to do is to move these ancillary code into presenters or decorators first.
If the code does indeed prove to be used in a lot of places, then I'd consider moving them into the model as a first class concern. Although this may be indicative of coupling that you can work to remove.
If this is more of an academic thing, I suggest refactoring now rather than later. Otherwise, it's really not a showstopper at this point. The associations look 
Joshua Summers
12:34, 28 Jul 20 (edit: 18:24, 28 Jul 20)
Agreed with Kent that this model isn't all that bad, but one improvement would be to move some of that `spkrs` code out of the model. At the very least, you can implement them as named scopes, but for me what I like to do is to move these ancillary code into presenters or decorators first.
OK cool, this is an area where I am totally lost as I don't really know much about named scopes, presenters or decorators. I've done some reading to try to understand and apply what you said. Here are my efforts:
Named Scopes:
I read: 
I am going to use this method as an example:
  def non_hidden_spkrs
    spkrs.where(hide: false).sort_by(&:created_at)
  end
My conclusion from reading the two posts was that the neatest solution would be:
# spkr.rb
class Spkr < ActiveRecord::Base
  scope :non_hidden, where(hide: false).sort_by(&:created_at)
end

# tlk.rb
calss Tlk < ActiveRecord::Base
  has_many :spkrs, dependent: :destroy

  def non_hidden_spkrs
    spkrs.non_hidden
  end
end
This is because it seems that if scoping on a has_many attribute of a model there is not a way to have the has_many association AND a scope within the parent (belongs_to) model (at least none of the examples have it). I could do:
# Tlk.rb
calss Tlk < ActiveRecord::Base
  has_many :spkrs, -> { where(hide: false).sort_by(&:created_at) }, dependent: :destroy
end
But this has two problems:
  1. When I call .spkrs I only get those where hide == false, but I want them all.
  2. When I destroy a Taaalk, it only destroys speakers where hide == false (see https://github.com/rails/rails/issues/26111)
Presenters & Decorators
I read:
This feels a lot more daunting to me than scopes! After reading about both presenters and decorators, presenters seemed to be the most friendly to me so I went with that (the third link got me to a not-too-intimidating place).
I would:
#app/presenters/tlk_presenter.rb
class TlkPresenter
  def initialize(tlk)
    @tlk = tlk
  end

  def non_hidden_spkrs
    @tlk.spkrs.where(hide: false).sort_by(&:created_at)
  end

  private

  attr_reader :tlk
end

# app/controllers/tlks_controller.rb
class TlksController < ApplicationController
  def show
    @tlk = Tlk.friendly.find(params[:id])
    @tlk_presenter = TlkPresenter.new(@tlk)
  end
end

# app/views/tlks/show.html.erb
<% @tlk_presenter.non_hidden_spkrs.each do |spkr| %>
  ...
<% end %>
Final Thoughts/Questions
Is my execution of these concepts right? And I feel like the use of scopes makes the most sense here and is neatest. Is that what you feel too?
Follow this Taaalk

5 followers

1,252 views

Start your own Taaalk, leave your details or meet someone to Taaalk with.