Design Principles

The code's arrangement is the design — Sandi Metz

It will need to change — Pau Pérez

Reduce the cost of change

Tell, don't ask Instead of having a dialog between objects, give simple and concise commands

Deciding on an object's behalf Not so good if file_connector.update_attributes(file_params) file_connector.migrate_to_provider! @file = file_connector.file_item render 'show' else raise AppException::UnprocessableEntity.new(file_connector.actionable_resource) end

A bit better if file_connector.update_and_migrate!(file_params) @file = file_connector.file_item render 'show' else raise AppException::UnprocessableEntity.new(file_connector.actionable_resource) end

Polymorphism Not so good def get_total_delay case email_type when 'incoming' (received_at - sent_at).to_i when 'outgoing' (received_at - created_on).to_i end end end

A bit better def total_delay email.total_delay end class IncomingEmail def total_delay received_at - sent_at end end class OutgoingEmail def total_delay received_at - created_on end end

Null Objects Not so good def frequency return 'none' unless master_task master_task.frequency_slug end

Equivalent to def frequency if master_task 'none' else master_task.frequency_slug end end

A bit better def frequency master_task.frequency_slug end class MasterTask def frequency_slug frequency_hash[:frequency].to_s end end class NoMasterTask def frequency_slug 'none' end end

Single Responsibility Principle

The S in SOLID

A class should have only one reason to change

Why should I care?

Clarity (Pablo Villalba 2009-09-02 | 2 # A User model describes an actual user, with his password and personal info. (Pablo Villalba 2009-09-02 | 3 # A Person model describes the relationship of a User that follows a Project.

class User < ActiveRecord::Base include User::Exceptions include SentientUser include Immortal include Digests include EmailBounces include Teambox::TestDeliveryEmail if Rails.env.development? include Teambox::Metadata metadata_store :settings metadata_store :metadata metadata_store :salesforce_contacts include User::Geoip include User::OutgoingEmail include User::Stats include User::FeatureTracking include User::ApiV2Mappings include Subscriptionable::User include Rails.application.routes.url_helpers include User::Ghost include OrganizationAdmins include User::Activation include User::Avatar include User::Calendars include User::Contacts include User::NewFromEmail include User::Roles include User::Rss include User::TaskReminders include User::Tokens include User::UserAuthentication include User::UserCallbacks include User::UserDeletion include User::UserFreezing include User::UserEmailers include User::UserFinders include User::UserNetwork include User::UserOauth include User::UserRelations include User::UserScopes include User::UserStats include User::UserTimeZones include User::UserValidations include User::UserApps include User::Invitations include User::DeprecatedColumns include Audited::User include Authentication include Authentication::ByPassword include Authentication::ByCookieToken include Chat::Concerns::UserChat # TODO: Please remove those accepts_nested_attributes_for :card attr_accessible :login, :username, :email, :first_name, :last_name, :biography, :password, :password_confirmation, :old_password, :time_zone, :locale, :first_day_of_week, :card_attributes, :notify_conversations, :notify_tasks, :notify_pages, :wants_task_reminder, :source, :newsletter, :project_activity_digest, :last_digest_delivery, :default_watch_new_task, :default_watch_new_conversation, :default_watch_new_page, :people_attributes, :google_calendar_url_token, :needs_profile, :settings, :metadata, :utm_source, :utm_medium, :utm_campaign, :utm_term, :bouncing_email, :phone_number_accessor, :country_accessor, :state_accessor, :wants_email_chat_notifications, :wants_chat_push_notifications, :job_title, :use_gravatar, :onboarding_tour_shown, :desktop_notifications_enabled attr_accessor :activate, :old_password, :invitation_token, :phone_number_accessor, :country_accessor, :state_accessor def to_s name end def to_param # in case it changes but is not yet saved login_was.match('.') ? id.to_s : login_was # Fix so user_path doesn't b0rk on dots in username end def locale if I18n.available_locales.map(&:to_s).include? self[:locale] self[:locale] else I18n.default_locale.to_s end end # TODO: Remove after APIv2 def person_for(project) self.people.find_by_project_id(project.id) end # TODO: Remove after APIv2 def member_for(organization) self.memberships.find_by_organization_id(organization.id) end def watching?(object) object.has_watcher? self end def in_project(project) project.people.find_by_user_id(self) end # Forces a password change # # @param [String] pass def change_password!(pass) self.password = pass self.password_confirmation = pass self.performing_reset = true save end # Checks if the user belongs to an org with enabled videoconference # # @return [Boolean] def can_use_videoconference? organizations.any? do |o| o.settings[:is_videoconference_banned] != true end end # Split a text in two parts and assign # to first_name and last_name # # @param [String] full_name def split_full_name(full_name) parts = full_name.strip.split(' ') return if parts.empty? self.first_name = parts.delete_at(0) self.last_name = parts.join(' ') if parts.present? end def crypted_password? self.crypted_password.present? end def short_name I18n.t 'common.format_name_short', first_name: first_name, last_name: last_name, first_name_first_character: first_name.chars.first, last_name_first_character: last_name.chars.first end end

Clarity $ find app/models/user/ -iname "*.rb" -type f -exec cat {} \; | wc -l 1887

Reusability app/models/file_item/providers_delegator.rb 230: def generate_public_token 240: def destroy_public_token app/models/folder.rb 72: def update_public_token 81: def generate_token app/models/hashtags.rb 38: def tokenize(str) app/models/signup.rb 15: def generate_token! 25: def create_unique_token app/models/upload.rb 183: def generate_public_token 193: def destroy_public_token 288: def random_token app/models/user/activation.rb 50: def set_login_token! 70: def generate_email_token! 87: def is_login_token_valid?(token) 121: def generate_email_token 126: def remove_email_token app/models/user/tokens.rb 10: def generate_token 19: def token_from_token(token) app/models/user/user_authentication.rb 41: def ensure_authentication_token 51: def generate_unique_authentication_token 61: def generate_random_token app/models/user/user_callbacks.rb 24: def update_token app/models/watcher.rb 16: def generate_token!

Reusability $ git grep SecureRandom.hex -- app/models/ | wc -l 11

Easier to test describe "domain" do it "should allow multiple organizations with nil domain" do create(:organization, domain: nil) expect(build(:organization, domain: nil)).to be_valid end it "shouldn't take a deleted organization's domain as busy" do o = create(:organization, domain: 'mofo.teambox.dev') o.destroy expect(build(:organization, domain: 'mofo.teambox.dev')).to be_valid end context 'when creating the organization' do let(:user) do super().update_attribute(:email, 'me@foobar.com') super() end it 'sets the organization domain' do expect(organization.domain).to eq('foobar.com') expect(organization.attributes['domain']).to eq('foobar.com') end end context 'when the domain is not set' do ... end end describe 'domain_from_creator' do subject { organization.domain_from_creator } context 'when the creator has a non-generic email' do let(:user) do super().update_attribute(:email, 'me@foobar.com') super() end it { is_expected.to eq('foobar.com') } context 'even if the creator has been deleted' do before do organization user.destroy end it { is_expected.to eq('foobar.com') }

Easier to test describe Domain, skip_truncation: true do let(:domain_string) { 'redbooth.com' } let(:domain) { described_class.new(domain_string) } describe '.from_user' do subject { described_class.from_user(user) } context 'when the user has supported email domain' do let(:user) { instance_double(User, email: 'pau.perez@redbooth.com') } it { is_expected.to be_a(Domain) } end context 'when the user has a non supported email domain' do let(:user) { instance_double(User, email: 'pau.perez@mailinator.com') } it 'raises an UnsupportedDomainError' do expect { described_class.from_user(user) }.to( raise_error(Domain::UnsupportedDomainError) ) end end end describe '.new' do context 'when the domain is blacklisted' do let(:domain_string) { 'yopmail.com' } it 'raises an UnsupportedDomainError' do expect { described_class.new(domain_string) }.to( raise_error(Domain::UnsupportedDomainError) ) end end context 'when the domain is allowed' do subject { described_class.new(domain_string) } it { is_expected.to be_a(Domain) } end context 'when the domain contains subdomains' do subject { described_class.new(domain_string) } let(:domain_string) { 'foo.bar.com' } it { is_expected.to be_a(Domain) } end end end

Easier to test $ b rspec spec/models/organization_spec.rb:118 Finished in 4.56 seconds 5 examples, 0 failures $ b rspec spec/models/domain_spec.rb Finished in 1.31 seconds 6 examples, 0 failures $ b rspec spec/models/schedule_spec.rb Finished in 1.4 seconds 24 examples, 0 failures

Easier to change def perform(started_at, *args) Teambox::ExceptionReport.rescue_and_reraise(self, { :queue => 'async_email', :args => args }) do if TEMPLATES_WITH_SEMAPHORE.include?(args[0]) delete_notify_delay_semaphore(*args) end OutgoingEmailer.deliver_template(*args.collect { |i| i.is_a?(Hash) ? i.with_indifferent_access : i }) end end def after_perform(started_at, *args) $statsd.timing "perf.queues.latency.#{@queue}", (Time.zone.now.to_f - started_at) * 1000 if Teambox.config.live_site? Teambox::Service.redis { |r| r.decr('emailer:pending') } end end private # Changes job queue depending on the mail template # @param template [String] template # # @return [Symbol] queue def queue_from_template(template) @queue = case template.to_sym when :signup_confirm_email, :reset_password, :forgot_password, :public_download :priority_urgent_async_mail when :notify_export, :notify_import, :daily_task_reminder, :bounce_message, :project_digest, :user_digest, :invalid_incoming_email :priority_low_async_mail else :async_mail end end

Cohesion

Every piece of a class should be strongly related to all the others

Low cohesion module ApplicationHelper IPhoneClients = /(iPhone|iPod)/i def redbooth_favicon def show_flash(*arguments) def iphone? def is_ios_app? def browser def mobile_platform def should_show_ios_modal? def should_show_android_modal? def location_name?(names) def location_name def loading_image(id) def loading(action, id = nil) def to_sentence(array) def errors_for(model, field) def rss? def signups_enabled? def cloud_edition? def cloud_link(path) end

Low cohesion class SupportTicket < ActiveRecord::Base CATEGORIES = ['question', 'complaint', 'bug', 'feedback', 'feature_request', 'billing_request', 'sales_question'] validates :category, presence: true validates :category, inclusion: { in: CATEGORIES } validates :question, presence: true validates :details, presence: true validates :user, presence: true belongs_to :user attr_accessible :category, :details, :issue_url, :question, :response attr_accessor :attachments before_create :submit_ticket! def subject "[#{RedboothProduct.current} - #{category}] #{question}" end def from "#{user.name} via #{Teambox.config.brand_name} <#{user.email}>" end private def submit_ticket! select_sender.submit_ticket end def select_sender case RedboothProduct.current when :cloud @sender = Support::ZendeskSender.new(user, self) when :private_cloud @sender = Support::MailSender.new(user, self) end @sender end end

High cohesion class Domain UnsupportedDomainError = Class.new(ArgumentError) def self.from_user(user) domain = user.email.split('@').last new(domain) end def initialize(domain) @domain = PublicSuffix.parse(domain) return unless BLACKLISTED_DOMAINS.include?(@domain.sld) raise UnsupportedDomainError, 'unsupported domain' end def to_s domain.domain end private attr_reader :domain end

High cohesion module User::Avatar extend ActiveSupport::Concern included do AvatarSizes = { :micro => [24, 24], :thumb => [48, 48], :profile => [278, 500] } has_attached_file :avatar, :url => (Teambox.config.amazon_s3 ? "/avatars/:id/:style.png" : "/system/avatars/:id/:style.png"), :default_url => "/assets/avatars/:style/missing.png", :path => (Teambox.config.amazon_s3 ? "avatars/:id/:style.png" : ":rails_root/public/system/avatars/:id/:style.png"), :styles => AvatarSizes.each_with_object({}) { |(name, size), all| all[name] = ["%dx%d%s" % [size[0], size[1], size[0] < size[1] ? '>' : '#'], :png] } attr_accessible :avatar, :avatar_destroy #validates_attachment_presence :avatar, :unless => Proc.new { |user| user.new_record? } validates_attachment_size :avatar, :less_than => 2.megabytes validates_attachment_content_type :avatar, :content_type => %w[image/jpeg image/pjpeg image/jpg image/png image/x-png image/gif image/bmp] end def cropping? !crop_x.blank? && !crop_y.blank? && !crop_w.blank? && !crop_h.blank? end def avatar_geometry(style = :original) @geometry ||= {} @geometry[style] ||= Paperclip::Geometry.from_file(avatar.path(style)) end # This is less performant than avatar_or_gravatar_url because it contacts S3 def avatar_or_gravatar_path(size, secure = false) (avatar? ? avatar.url(size) : gravatar(size, secure)).tap do |avatar_url| avatar_url.sub! 'http:', 'https:' if secure end end

Low cohesion lass Task < RoleRecord STATUS_NAMES = [:new, :open, :hold, :resolved, :rejected] # values equal or bigger than :resolved will be considered as archived tasks STATUSES = STATUS_NAMES.each_with_index.each_with_object({}) {|(name, code), all| all[name] = code } ACTIVE_STATUS_CODES = [:new, :open].map { |name| STATUSES[name] } UNARCHIVED_STATUS_CODES = [:new, :open, :hold].map { |name| STATUSES[name] } ARCHIVED_STATUS_CODES = [:resolved, :rejected].map { |name| STATUSES[name] } include Immortal include Teambox::Metadata include ConciseComments include MentionScanner include Task::TaskRelations include Task::TaskValidations include Task::TaskScopes include Task::TaskCallbacks

something changes often => many responsibilities

too many responsibilities => low cohesion

Tools $ reek app/models $ flog -g -d app/models $ churn -c 10 --start_date '1 year ago'

Resources • Practical Object-Oriented Design in Ruby by Sandi Metz • Design Patterns in Ruby by Russ Olsen • Upcase by Thoughtbot

Conclusions

It's about OOP, not Rails

be aware of the coupling