GCorbel

I have some questions :

In your opinion, what is the responsability of an ActiveRecord model? It is a data object which manage the validations and the persistence.

In your example, what do you think about a creator like this :

class UserCreatorService
  def initialize(user)
    @user = user
  end
  
  def create
    @user.token = TokenGenerator.new.generate
    @user.create
    UserMailer.subscription(@user).deliver
  end
end

Is it correct? What if we need to association some models, do we must to pass params to the class and the class is responbile to build other models or do we must pass models in parameter and build them elsewhere (in the controller for exemple)?

How do you will refactor this class? This is a central part of my application and it’s related to many other models. It belongs to many things and it has many things. There is methods which talk about images and currencies but I don’t know where to put them to simplify my class. Any solution?

Thanks! Another great video!

1 reply

bowton

Great episode I look forward to watching episodes on the remaining principles of sOLID.

I have a question that is not directly related to the subject of this episode but the example used for the tell don’t ask principle.

Is the example in this episode really a violation of the Tell don’t Ask principle? In the example the view is being rendered within the context of a ViewObject where @account is an instance variable within it’s scope. The check of the accounts state to conditionally render some content including more account state seems to be view logic combined with view data (the account).

<% if @account.invitations_remaining? %>
  <p>
    You have
    <span class="count">
      <%= @account.invitations_remaining %>
    </span>
    remaining.
    <%= link_to 'Invite?', new_invitation_path %>
  </p>
<% else %>
  <p>
    You have no invitations remaining.
    <%= link_to 'Upgrade?', edit_plan_path %>
  </p>
<% end %>

My understanding of TDA is that it would be a violation if the view asked account about its state and then preformed some logic on account depending on the response.

<% if @account.invitations_remaining? %>
  <p>
    You have
    <span class="count">
      <%= @account.total_invitations - @account.invitations_used %>
    </span>
    remaining.
    <%= link_to 'Invite?', new_invitation_path %>
  </p>
<% else %>
  <p>
    You have no invitations remaining.
    <%= link_to 'Upgrade?', edit_plan_path %>
  </p>
<% end %>

ralphos

I’d just like to chime in on the discussion, since I’m also interested in learning more about SRP at the moment. Please correct me if I’m wrong about anything!

@Guirec_Corbel - in your example, from what I’ve read about Service classes, a general rule may be to not create Service classes that deal with simple CRUD operations for your model. ActiveRecord Models should be responsible for validations and persistence and CRUD may fall within this scope.

With that said, your Service class doesn’t appear to be a simple CRUD operation, so it may be worthwhile re-naming your Service class and method to not confuse others. This appears to be a good opportunity to use the Command Pattern to be responsible for encapsulating all of the logic required for the interaction between User, TokenGenerator and UserMailer?

In a previous post, @derekprior mentioned having a /processes folder to include classes dealing with business processes. This also reminds me of this post using a form object, which you could put in a /processes directory or /models directory.

You could characterize this interaction as a domain service (as per the definition in Domain-Driven Design), since it’s a use-case involving multiple domain objects. So I guess whether you call it a Service object, a form object or a generic PORO is up to you. A form object called Registration, seems like the most straightforward way, although some people like to append the name of the pattern they’re using in the class name itself.

Thoughts anyone else?

@bowton I think when they were referring to “Tell, don’t ask” the violation they were referring to is in this line:

<% if @account.invitations_remaining? %>

In other words, @account is being asked, “Hey do you have any invitations remaining?” as opposed to just telling @account, give me your invitations remaining, whether you have some or not. (The not being an implementation of perhaps the NullObject pattern, which seems to be the go to solution in these cases).

1 reply

GCorbel

@ralphwintle, so, if I understand as well, you suggest to do something like this :

In the controller :

def new
  @form = User.new
end

def create
  command = CreateUserRegistration.new(params[:user])
  if command.valid?
    comment.process
    # ...
  else
    # ...
  end
end 

In a process file :

class CreateUserRegistration
  def initialize(params)
    @params = params
  end
  
  def process
    user = User.new(params)
    user.token = TokenGenerator.new.generate
    user.create
    UserMailer.subscription(user).deliver
  end
end

Basicaly, it’s just rename class and methods and put the file in another folder, right?

I agree with the creation of a form object. I just have a single question. Where to create model. Should it be in the form object like this :

class UserRegistrationForm
  def initialize(params)
    @params = params
  end
 
  def user
    @user ||= User.new(params_for_user)
  end

  def associated_model
    @associated_model ||= AssociatedModel.find(param_to_find_the_model)
  end
end

I don’t think is the right place. It will become too complicated for complexe relations. What you think?

@jferris and @benorenstein, It could be a great subject for a weekly iteration.

1 reply

samnang

Another great episode. It makes a lot of senses when you pull out token generator out of invitation. I’m waiting until the end of episode to see how you put them work together? Do you use form object or command object to create invitation to tight them together?

@jferris @benorenstein could you put the whole after refactored version example in a gist?

Dreamr

I create service objects like this all the time. But code to interface and use dependency injection. Also I prefer verbs to nouns on these single purpose classes. Nouns are a junk drawer waiting to happen, verbs are SRP friendly. I also prefer single method objects to use CALL as the method interface so it is interchangeable with procs and lambdas.

class UserCreator
  def initialize(user)
    @user = user
  end

  def call(token_generator, mailer)
    @user.token = token_generator.generate
    @user.create
    mailer.deliver(@user)
  end
end

To me AR classes are just for db and validation, but there is no reason you cannot abstract further. Just don’t mix duties.

1 reply

Dreamr

Use dependency injection.

def initialize(model, params)
  @model = model
  @params = params
end

def user
  @user ||= @user_model.new(params.fetch(:user))
end

samnang

@Dreamr so in UserCreator example, your controller will instantiate user model and check validation before you instantiate UserCreator with all dependencies, so how do you test your controller because what I used to worry about is we have to stub many things to be able to test controller’s action?

1 reply

Dreamr

I don’t write controller tests. I move the non database logic to a CommandObject (one for each action) and I keep the database stuff in the AR model. I don’t test database interaction either. I take DHH very seriously when he says ‘Don’t test Rails, it is tested’.

My create method would look like this:

def create
  if UserCreator.call(User, params).success?
    ...
  else
    ...
  end
end

Then I wrap all the logic for checking and creating a user there and chain in a success? or failed? from there.

This means the testing moved from the controller to the PRO (pure ruby object).

I don’t test controllers, I don’t test validations, I don’t test the databse. I take DHH very seriously when he says “Don’t test rails internals, they are already tested”

maciejtomaka

class Account
  def remaining_invitations
   if remaining_invitations_count.zero?
    RemainingInvitations.new(remaining_invitations_count)
   else
     AccountInvitiationsDepleted.new(0)
   end
  end
end

class RemainingInvitations < Integer
end

class DepletedRemainingInvitations < Integer
end

Separate partials named _remaining_invitations and _depleted_remaining_invitiations would be rendered as:
render @account.remaining_invitations

It makes you don’t ask but tell and don’t have html in Account class. But it is too much to have such a method in Account, maybe a decorator is better place for that?

What you think?

1 reply

ralphos

So couple things @maciejtomaka.

In my opinion, you’re still violating tell don’t ask here by having a remaining_invitations method. Whist it’s not a question, remaining_invitations does sound like it’s sniffing around your invitations - and indeed inside the method you’re actually asking for a count/zero…

I’m not sure if RemainingInvitations or DepletedRemainingInvitations classes are needed (not digging them at all to be honest) and having them inherit from Integer feels kinda wrong.

I like the idea of rendering your invitations with a partial, so to answer your question - since you want to ‘tell, don’t ask’ and use a partial to display invitations in your view, how about the following?

class Account < ActiveRecord::Base
  has_many :invitations
end

and in your view:

<%= render @account.invitations %>

Bit of Rails magic going on here but basically this is going to loop over the invitations collection, find an _invitation.html.erb partial and pass in an invitation variable.

1 reply

maciejtomaka

<%= render @account.invitations %>

You will render a list of invitations. You have to render something else here. It is actually an information about remaining_invitations count. I have just added the meaning to 0 and positive integers by using a class for it.
If you reneder then it renders one of two templates.

Rendering a collection is easy as you said it is rails magic. I do it the same, but I add separate meaning for 0 and positive values.

Two possible outputs:

1. you have X invitations left,
2. Upgrade for more invitiations.

1 reply

ralphos

IMO there’s an unnecessary level of indirection in your example.

I get confused when you ask if remaining_invitations_count is zero and then pass in the count to RemainingInvitations. Shouldn’t RemainingInvitations be able to tell you how many remaining invitations it’s got?

Creating an object (AccountInvitationsDepleted) to represent a null state for Invitations is OK, but why is it inheriting from Integer? What about applying the null object pattern here?

At the end of the day your view simply requires 1 of 2 options to be shown.

<% if invitations.empty? %>
  # Upgrade for more invitations
<% else %>
  # You have X invitations left
<% end %>

Do you need anything else? At some point in your code you’re going to need to use conditional logic. In this case, it’s not that bad to keep it in the view IMO.

Even if you put both scenarios into one partial and the partial was smart enough to show the correct information - what would you call that partial?

1 reply

maciejtomaka

Inheriting from integer is weir - you are right. AccountInvitationsDepleted should hold information about invitations - in this example remaining_invitations_count should be anough.

The idea is to add more meaning to simple value.
Maybe this expample is too trival to see a gain.
Please imagine tree situations:
0 - Upgrade for more invitations
1…3 - Your invitations will soon deplete
4+ - Simply show the invitations

We have just named the states by using the classes and it is easy to render in view - just 3 partials with zero conditionals.
There are more situations when there is also user input involved (second variable).

croaky

In this episode, this Rails view code was shown as an example of tension between the patterns “Tell, Don’t Ask” and “Single Responsibility Principle”:

<% if @account.invitations_remaining? %>
  <p>
    You have
    <span class="count">
      <%= @account.invitations_remaining %>
    </span>
    remaining.
    <%= link_to "Invite?". new_invitation_path %>
  </p>
<% else %>
  <p>
    You have no invitations remaining.
    <%= link_to "Upgrade?", edit_plan_path(@account.plan) %>
  <p>
<% end %>

I’m curious whether folks feel that a JavaScript-based view+template solution helps respect both patterns. See the following Backbone example.

invitations-remaining-view.coffee:

define [
  "backbone"
  "./invitations-remaining-view.tmpl"
], (
  Backbone
  template
) ->

  class InvitationsRemainingView extends Backbone.View

    render: ->
      super
      @$el.html template(invitations_remaining: @model.get("invitations_remaining"))
      this

    shouldBeDisplayed: ->
      @model.get("invitations_remaining") > 0

invitations-remaining.handlebars:

<p>
  You have <span class="count">{{ invitations_remaining }}</span> remaining.
  <a href="/invitations/new">Invite?</a>
</p>

no-invitations-remaining-view.coffee:

define [
  "backbone"
  "./no-invitations-remaining-view.tmpl"
], (
  Backbone
  template
) ->

  class NoInvitationsRemainingView extends Backbone.View

    render: ->
      super
      @$el.html template(plan_id: @model.get("plan_id"))
      this

    shouldBeDisplayed: ->
      @model.get("invitations_remaining") == 0

no-invitations-remaining-view.handlebars:

<p>
  You have no invitations remaining.
  <a href="/plans/{{ plan_id }}/edit">Upgrade?</a>
<p>

There might be a wrapper view like invitations-view.coffee:

define [
  "backbone"
  "./invitations-view.tmpl"
], (
  Backbone
  template
) ->

  class InvitationsView extends Backbone.View

    initialize: ->
      super
      @_removeDisplayPolicy()

    render: ->
      @$el.html template
      @_renderDisplayPolicy()
      this

    remove: ->
      super
      @_removeDisplayPolicy()

    _renderDisplayPolicy: ->
      unless @displayPolicy?
        @displayPolicy = new SectionDisplayPolicy({
          el: @$('.sections')
          model: @model
          sectionViews:
            '.invitations-remaining-container': InvitationsRemainingView
            '.no-invitations-remaining-container': NoInvitationsRemainingView
        }).render()

    _removeDisplayPolicy: ->
      @displayPolicy?.remove()
      @displayPolicy = undefined

invitations-view.handlebars:

<article class="sections">
  <div class="invitations-remaining-container"></div>
  <div class="no-invitations-remaining-container"></div>
</article>

That SectionDisplayPolicy object is the secret sauce. Different JavaScript frameworks may have something like this built in, or there may be some great plugins that do something similar, but for the purpose of example so you can read an implementation, here’s one that @seangriffin and I wrote:

define [
  "lodash"
  "backbone"
], (
  _
  Backbone
) ->
  # SectionDisplayPolicy is responsible for managing the sections of a page by
  # rendering and removing them from the DOM.
  class SectionDisplayPolicy extends Backbone.View

    initialize: (options) ->
      @sectionViews = _.mapValues options.sectionViews, (View) =>
        new View(model: @model)

      @hiddenSections = _.values(@sectionViews)
      @displayedSections = []
      @listenTo @model, "change", @_updateSections

    render: ->
      @_updateSections()
      this

    remove: ->
      @stopListening()

      _.each @displayedSections, (section) ->
        section.remove()

      this

    _updateSections: =>
      @_removeSections()
      @_renderSections()

    _renderSections: ->
      _.each @hiddenSections, (section) =>
        if section.shouldBeDisplayed()
          @_containerFor(section).html(section.render().el)
          @displayedSections.push(section)
          @hiddenSections = _.without(@hiddenSections, section)

    _removeSections: ->
      _.each @displayedSections, (section) =>
        unless section.shouldBeDisplayed()
          section.remove()
          @hiddenSections.push(section)
          @displayedSections = _.without(@displayedSections, section)

    _containerFor: (target) ->
      selector = _.findKey(@sectionViews, target)
      @$(selector)

This style has many small objects that use the shouldBeDisplayed interface defined by the SectionDisplayPolicy object to contain the knowledge of when to display themselves based on the model’s data.

There is no conditional logic in the template (no logic at all, actually), no conditional methods defined on the model (it can be a dumb data container) and I believe it does not violate “Single Responsibility Principle” or “Tell, Don’t Ask”, but I’d love to hear others’ opinions.

There is more code to write in this version, but the majority of the logic is in a library object, SectionDisplayPolicy, which is very versatile. We’re using it heavily in our current Backbone app. The rest of the code is very boiler-plate-y and straightforward. It’s using Require.js to define the dependencies and the bare minimum necessary for a Backbone.View to do its work.

samnang

@jferris @benorenstein When pull tokenize functionality into its own class TokoenizedModel to follow SRP, so it means in our controller actions like create we have to wrap AR model with TokoenizedModel before we call save it or in index action we have to pull AR::Relation and map those objects with TokoenizedModel before we render views that depend token functionality.

But my question should controllers are right place that have that knowledge or should be somewhere else?