Ben and Joe cover "S" from SOLID, which stands for "Single Responsibility Principle," often abbreviated as "SRP."
We show examples of why you might choose to obey this principle, as well as how the principle relates to [cohesion], Tell, Don't Ask...
This is a companion discussion topic for the original entry at https://thoughtbot.com/upcase/videos/single-responsibility-principle
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!
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 %>
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).
2 Likes
@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.
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?
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.
Use dependency injection.
def initialize(model, params)
@model = model
@params = params
end
def user
@user ||= @user_model.new(params.fetch(:user))
end
@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?
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ā
1 Like
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?
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.
<%= 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:
- you have X invitations left,
- Upgrade for more invitiations.
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?
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).
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.
@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?