-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RailsConf: Business logic in model #15
base: rc-controller-concerns
Are you sure you want to change the base?
Conversation
* Profanity check is called before model saved for a couple reasons: 1. If we save the micropost, that creates a validation message which would duplicate the profanity flash. 2. A bit more work confirm that validation error on the content was profanity without checking the message. * Added validation test.
* User has method to send parent notification of profanities used. * Micropost gets method save_with_profanity_callbacks which updates the user model if needed. * Custom flash message not handled and controller spec marked "pending"
* Removed pending for controller spec
* Created User#update_for_using_profanity(profane_words_used) * Simplfied Micropost#save_with_profanity_callbacks
This slims up the MicropostController create action method.
* Created private method adjust_micropost_profanity_message
@@ -53,4 +27,11 @@ def correct_user | |||
@micropost = current_user.microposts.find_by(id: params[:id]) | |||
redirect_to root_url if @micropost.nil? | |||
end | |||
|
|||
def adjust_micropost_profanity_message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
call it nitpicky, but I hate that method name. Perhaps set_micro.... I know it isnt much of a change, but adjust is such a bland, non-descriptive word to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd buy that. I should have called it "replace_micropost_profanity_message".
Please don't take offense by my comments here. If you would like to pair on this today a bit to get my input I have some time. Let me know. |
Comments very much appreciated! |
No problem amigo. I love to talk about esoteric code quality. On Sun, Apr 20, 2014 at 11:29 AM, Justin Gordon notifications@git.luolix.topwrote:
James Advanced Ruby musings @ RubyLove.io Expert mentor @ Codementor.io |
# This could have been done with and after_save hook, but it seems wrong to ever be sending | ||
# emails from after_save. | ||
# Return true if save successful | ||
def save_with_profanity_callbacks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am curious why the additional save method instead hooking into the normal AR save on the model? To me, this doesn't scream 'obvious' api.
All things equal here, I would hook this in as a private to save.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't like hooking into save anything that does things like sends notifications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the problem with this approach is that the rules we want to enforce will be skipped, if I forget to call the magic save method (read: follow normal ActiveRecord practices).
I do agree the hook should not send the email. Really, the model shouldn't either, in my opinion. The controller should send the email, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also still about the validation (which I extracted for you), so the point is moot and this code would not exist here! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the after_save hook is correct and keeping the notification code in the controller, as that should be the only external entry point.
From the docs: ActiveRecord::Transactions::ClassMethods
save and destroy are automatically wrapped in a transaction
Both save and destroy come wrapped in a transaction that ensures that whatever you do in validations or callbacks will happen under its protected cover. So you can use validations to check for values that the transaction depends on or you can raise exceptions in the callbacks to rollback, including after_* callbacks.
I really like the changelog in the PR top. Excellent! |
That's all the :ruby: ❤️ I have for you at the moment, good luck! |
You and I are also on opposite ends of the spectrum - I would take issue with almost all of these: Use Rails constructs first. Don't invent patterns that don't need to be invented. Lets pair after you are back from the conf and your talk is over and I will win you over to the right side with evidence, and proof. MVC is SEVERELY lacking in scope. We used to have fat views. The problem is MVC isn't enough. That is an abstraction ONLY FOR THE WEB. Your app may have a WEB interface, but it is NOT a WEB APP. It is whatever your domain is, app. With a web front end. That means build our YOUR DOMAIN IN RUBY, and not constantly bring rails into it. Your use of :) with ❤️ of course |
For what it's worth, I like this example the best of what I've seen so far. |
def save_with_profanity_callbacks | ||
transaction do | ||
valid = save | ||
if profanity_validation_error? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this also goes away after the validation is extracted out.
@thatrubylove, per your comment: You and I are also on opposite ends of the spectrum - I would take issue with almost all of these. I think just as a view needs to consider the context of html or api, the choice of how one uses the Rails framework needs to consider the context. I'm a freelancer, so I expect other Rails programmers to collaborate on my Rails code. About the only assumption I can make about these programmers is that they should understand Rails constructs. OTOH, if you're in a single company, and want to consciously deviate from the Rails defaults, that's more feasible. That being said, I look forward to exploring how we'd change this example to fit your criteria. |
ok, I will do the code :) give me a bit though, as I am writing a lib at the moment to create an On Sun, Apr 20, 2014 at 1:52 PM, Justin Gordon notifications@git.luolix.topwrote:
James Advanced Ruby musings @ RubyLove.io Expert mentor @ Codementor.io |
RailsConf 2014 Talk, Tuesday, 2:30, Ballroom 4, Concerns, Decorators, Presenters, Service Objects, Helpers, Help Me Decide!
Use Rails rather than Service Objects
Refactored fat, complicated, full-of-business MicropostController create action so that business logic resides in models.
rather not invoke complicated business logic from a hook.
Summary
the Model layer.
up many instance variables in the controller (see
http://parley.rubyrogues.com/t/presenters-refactoring-example/1988).
include/extend when callbacks and static methods are involved.
controllers.
to group related models.
controller to the view.
Feature Story
Here's the code that implements the business rules:
If a minor posts profane words:
Code to clean up
The starting point code below is 100% functional in the application. It has good
tests. There is nothing semantically wrong. The problem is that just the code is
in the wrong place, as the controller's responsibility is the interaction
between the view and the model.
Refactoring Story
I started out this exercise to come up with a good example of Service Objects,
as described in 7 Patterns to Refactor Fat ActiveRecord Models. To quote it, the
criteria for Service Objects:
period)
Order, CreditCard and Customer objects)
networks)
outdated data after a certain time period).
With that in mind, I created this example, and I refactored into "Service
Objects" as shown in this pull request for Service Objects. The main criticism
of this code was that it was too close to the controller. In other words,
there's a lot of extra code just to have the code logic outside of the
controller.
I then did the extreme opposite of creating a controller class with only one
method, as shown in this pull request for Single Purpose Controller. That
example had the reverse problem having all the business logic in the controller.
The net result is this pull request, which doesn't show you how to break code
into a "Service Object" but instead shows that you don't necessarily need to do
so when you move the code to the right places in your existing models.
The final controller code looks like this. It meets our criteria for small
method size, and all the code is in the "right place".
Refactoring Steps
Refactorings are best done in small steps, while running unit tests.
Move profanity validation from controller to model
reasons:
would duplicate the profanity flash message
profanity without checking the message.
Move Micropost profanity checking logic to models
save_with_profanity_callbacks
which updates theuser model if needed.
Fix custom flash message for micropost profanity
twice.
Further move logic into User model
User#update_for_using_profanity(profane_words_used)
Micropost#save_with_profanity_callbacks
Move
profanity_violation_message
toMicropostDecorator
This slims up the MicropostController create action method. Supposing another
area of the code needed this message, having the message on the Draper decorator
makes it easier to find.
Slim MicropostsController create action
adjust_micropost_profanity_message
to slim down thesize of the controller action.
References
Special Thanks!
This example has been improved by comments from:
@dhh, @JEG2, @gylaz, @jodosha, @dreamr, @thatrubylove, @therealadam, @robzolkos