RailsConf: Business logic in model

[justin808]

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.

• Moved validation to Micropost from Controller
• Created method Micropost#save_with callbacks__profanityto handle the business logic aspects.
• Considered putting this logic in the after_save hook for Micropost, but I'd rather not invoke complicated business logic from a hook.

Summary

• Use Rails constructs first. Don't invent patterns that don't need to be invented.
• Think in terms of domains, not patterns or other "technical" refactorings. At the same time, strive for:
• Small methods
• Small classes
• Clarity
• Simplicity
• The key problem is putting code in the right place:
• Models: business logic goes here, and doesn't have to be an AR model.
• Controller: Interaction code goes here. Try to move any business logic into the Model layer.
• Presenter: Encapsulate preparation of objects for view rather than setting up many instance variables in the controller (see http://parley.rubyrogues.com/t/presenters-refactoring-example/1988).
• Have a good toolbox for good domain based Refactoring including:
• Use Concerns, for both model and controller, which are better than include/extend when callbacks and static methods are involved.
• For non CRUD based controller actions, create new controllers. Avoid huge controllers.
• Don't be afraid to create non AR based models. Use modules within app/models to group related models.
• Use Rails validations.
• Use the Presenters pattern of having a object wrap state going from the controller to the view.

Feature Story

Here's the code that implements the business rules:

If a minor posts profane words:

1. The post shall not be valid.
2. A counter will track how many times the minor tried to use profanity.
3. The minor's parents shall be notified.
4. A special flash alert will alert the minor to profanity usage.

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.

    class MicropostsController < ApplicationController
      before_action :signed_in_user, only: [:create, :destroy]

      def create
        @micropost = Micropost.new(micropost_params.merge(user: current_user))
        if current_user.minor? && (profane_words_used = profane_words_in(@micropost.content))
            current_user.increment(:profanity_count, profane_words_used.size)
            current_user.save(validate: false)
            send_parent_notifcation_of_profanity(profane_words_used)
            flash.now[:error] = <<-MSG.html_safe
              <p>Whoa, better watch your language! Profanity: '#{profane_words_used.join(", ")}' not allowed!
              You've tried to use profanity #{view_context.pluralize(current_user.profanity_count, "time")}!
              </p><p class="parent-notification">Your parents have been notified!</p>
            MSG
            render 'static_pages/home'
        else
          if @micropost.save
            flash[:success] = "Micropost created!"
            redirect_to root_url
          else
            render 'static_pages/home'
          end
        end
      end

      private

        # return array of profane words in content or nil if none
        def profane_words_in(content)
          # PRETEND: Hit external REST API

          # NOTE: Implementation below is a simulation
          profane_words = %w(poop fart fartface poopface poopbuttface)
          content_words = content.split(/\W/)
          content_words.select { |word| word.in? profane_words }.presence
        end

        def send_parent_notifcation_of_profanity(profane_words)
          # PRETEND: send email
          Rails.logger.info("Sent profanity alert email to parent of #{current_user.name}, "\
              "who used profane words: #{profane_words}")
        end

        def micropost_params
          params.require(:micropost).permit(:content)
        end
    end

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:

• The action is complex (e.g. closing the books at the end of an accounting period)
• The action reaches across multiple models (e.g. an e-commerce purchase using Order, CreditCard and Customer objects)
• The action interacts with an external service (e.g. posting to social networks)
• The action is not a core concern of the underlying model (e.g. sweeping up 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".

    class MicropostsController < ApplicationController
      before_action :signed_in_user, only: [:create, :destroy]

      def create
        @micropost = Micropost.new(micropost_params.merge(user: current_user))
        if @micropost.save_with_profanity_callbacks
          flash[:success] = "Micropost created!"
          redirect_to root_url
        else
          adjust_micropost_profanity_message
          render 'static_pages/home'
        end
      end

      private
        def micropost_params
          params.require(:micropost).permit(:content)
        end

        def adjust_micropost_profanity_message
          if @micropost.profanity_validation_error?
            @micropost.errors[:content].clear # remove the default validation message
            flash.now[:error] = @micropost.decorate.profanity_violation_msg
          end
        end
    end


    class Micropost < ActiveRecord::Base
      belongs_to :user
      validates :content, presence: true, length: { maximum: 140 }
      validates :user_id, presence: true
      validate :no_profanity

      def self.from_users_followed_by(user)
        # omitted for clarity
      end

      # return array of profane words in content or nil if none
      def profane_words_in_content
        # PRETEND: Hit external REST API

        # NOTE: Implementation below is a simulation
        profane_words = %w(poop fart fartface poopface poopbuttface)
        content_words = content.split(/\W/)
        content_words.select { |word| word.in? profane_words }.presence
      end

      # 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
        transaction do
          valid = save
          if profanity_validation_error?
            user.update_for_using_profanity(profane_words_in_content)
          end
          valid
        end
      end

      def profanity_validation_error?
        errors[:content].find { |error| error =~ /\AProfanity:/ }
      end

      private
        def no_profanity
          if user && user.minor? && (profane_words = profane_words_in_content)
            errors.add(:content, "Profanity: #{profane_words.join(", ")} not allowed!")
          end
        end
    end

    class User < ActiveRecord::Base

      def update_for_using_profanity(profane_words_used)
        increment(:profanity_count, profane_words_used.size)
        save(validate: false)
        send_parent_notification_of_profanity(profane_words_used)
      end

      def send_parent_notification_of_profanity(profane_words)
        # PRETEND: send email
        Rails.logger.info("Sent profanity alert email to parent of #{name}, "\
              "who used profane words: #{profane_words}")
      end
    end

Refactoring Steps

Refactorings are best done in small steps, while running unit tests.

Move profanity validation from controller to model

• Makes sense because we never want to persist an invalid model object.
• However, the 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 message
2. A bit more work confirm that validation error on the content was profanity without checking the message.

Move Micropost profanity checking logic to models

• Micropost gets method save_with_profanity_callbacks which updates the user model if needed.
• User has method to send parent notification of profanities used.
• Custom flash message not handled and controller spec marked "pending"

Fix custom flash message for micropost profanity

• Need to clear validation message on the model to prevent message showing twice.
• Removed pending for controller spec

Further move logic into User model

• Created User#update_for_using_profanity(profane_words_used)
• Simplfied Micropost#save_with_profanity_callbacks

Move profanity_violation_message to MicropostDecorator

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

• Created private method adjust_micropost_profanity_message to slim down the size of the controller action.

References

1. RailsGuides: Yes, know Rails well first before going to other patterns.
2. 7 Patterns to Refactor Fat ActiveRecord Models
3. DHH Documents Example of 2 Controllers with Controller Concern
4. Thoughtbot's Learn program, especially the Learn Forum
5. Ruby Rogues Parley Forum

Special Thanks!

This example has been improved by comments from: @dhh, @jeg2, @gylaz, @jodosha, @dreamr, @thatrubylove, @therealadam, @robzolkos

[thatrubylove]

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.

[justin808]

Comments very much appreciated!

[thatrubylove]

No problem amigo. I love to talk about esoteric code quality.

On Sun, Apr 20, 2014 at 11:29 AM, Justin Gordon [email protected]:

Comments very much appreciated!

— Reply to this email directly or view it on GitHubhttps://github.com/justin808/fat-code-refactoring-techniques/pull/15#issuecomment-40898657 .

James @thatrubylove

Advanced Ruby musings @ RubyLove.io https://www.rubylove.io

Expert mentor @ Codementor.io https://www.codementor.io/thatrubylove

[thatrubylove]

I really like the changelog in the PR top. Excellent!

[thatrubylove]

That's all the :ruby: :heart: I have for you at the moment, good luck!

[thatrubylove]

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. Think in terms of domains, not patterns or other "technical" refactorings. At the same time, strive for: Small methods Small classes Clarity Simplicity The key problem is putting code in the right place: Models: business logic goes here, and doesn't have to be an AR model. Controller: Interaction code goes here. Try to move any business logic into the Model layer. Presenter: Encapsulate preparation of objects for view rather than setting up many instance variables in the controller (see http://parley.rubyrogues.com/t/presenters-refactoring-example/1988). Have a good toolbox for good domain based Refactoring including: Use Concerns, for both model and controller, which are better than include/extend when callbacks and static methods are involved. For non CRUD based controller actions, create new controllers. Avoid huge controllers. Don't be afraid to create non AR based models. Use modules within app/models to group related models. Use Rails validations. Use the Presenters pattern of having a object wrap state going from the controller to the view.

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. Then we had fat controllers. Now we have fat models. The MVCers have run out of places to hide.

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 .in? over `.include?isConcernerningas is your use ofConcerns``

:) with :heart: of course

[JEG2]

For what it's worth, I like this example the best of what I've seen so far.

[justin808]

@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.

[thatrubylove]

ok, I will do the code :)

give me a bit though, as I am writing a lib at the moment to create an attr_accessor_f that instead of creating a macro that allows mutation, it instead creates a macro that when any attribute is altered, it will return a new, frozen instance with the new value and a new object_id, a la functional!

On Sun, Apr 20, 2014 at 1:52 PM, Justin Gordon [email protected]:

@thatrubylove https://github.com/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.

— Reply to this email directly or view it on GitHubhttps://github.com/justin808/fat-code-refactoring-techniques/pull/15#issuecomment-40901908 .

James @thatrubylove

Advanced Ruby musings @ RubyLove.io https://www.rubylove.io

Expert mentor @ Codementor.io https://www.codementor.io/thatrubylove