Upgrade to Pro — share decks privately, control downloads, hide ads and more …

Design Principles

Pau Pérez
December 31, 2015

Design Principles

Basic object-oriented design principles for Rails applications: tell don't ask, single responsibility principle and cohesion.

Pau Pérez

December 31, 2015
Tweet

More Decks by Pau Pérez

Other Decks in Programming

Transcript

  1. 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
  2. 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
  3. 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
  4. 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
  5. Null Objects Not so good def frequency return 'none' unless

    master_task master_task.frequency_slug end
  6. 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
  7. 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.
  8. 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
  9. 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!
  10. 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, '[email protected]') 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, '[email protected]') 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') }
  11. 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: '[email protected]') } 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: '[email protected]') } 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
  12. 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
  13. 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
  14. 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
  15. 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
  16. 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
  17. 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
  18. 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
  19. Tools $ reek app/models $ flog -g -d app/models $

    churn -c 10 --start_date '1 year ago'
  20. Resources • Practical Object-Oriented Design in Ruby by Sandi Metz

    • Design Patterns in Ruby by Russ Olsen • Upcase by Thoughtbot