Brown bag 1
Twitter: @krzkot
Github: lis2
Email: kotlarek.krzysztof@gmail.com
- Performance
- Maintainability
what is needed is a mathematical technique that will provide a quantitative basis for modularization and allow us to identify software modules that will be difficult to test or maintain.
1976, Thomas J. McCabe
Cyclomatic Complexity Metric
Algorithm that counts the number of unique execution paths through a body of source code
Code also assigns values to variables and sends messages
Jerry Fitzpatrick published in 1997 "Applying the ABC Metric to C, C++, and Java"
- Assignments is a count of variable assignments.
- Branches counts not branches of an if statement (as one could forgivably infer) but branches of control, meaning function calls or message sends.
- Conditions counts conditional logic.
There should never be more than one reason for a method to change.
- If you can’t tell exactly what a method does at a glance, it’s too long.
- Methods with more than one level of nesting are usually too long.
- Methods with a flog score of 10 or higher may be too long.
class UserService
def self.create_user!(registration_request)
ActiveRecord::Base.transaction do
is_new_record = !User.exists?(email: registration_request[:email])
user = user_from_registration_request! registration_request
return user, is_new_record
end
end
end
def create
user, is_new_record = UserService.create_user!(user_hash)
unless(is_new_record)
return(render_unprocessable_entity('User already exists.'))
end
...
end
def self.exists?(registration_request)
!User.exists?(email: registration_request[:email])
end
def self.create_user!(registration_request)
ActiveRecord::Base.transaction do
user_from_registration_request! registration_request
end
end
def create
unless UserService.exists?(user_hash)
return(render_unprocessable_entity('User already exists.'))
end
user = UserService.create_user!(user_hash)
...
end
A class should have only one reason to change.
Martin Fowler
- You can’t easily describe what the class does in one sentence.
- You can’t tell what the class does without scrolling.
- The class needs to change for more than one reason.
- The class has more private methods than public methods.
- The class has more than 7 methods.
- The class has a total flog score of 50.
- The class is changed more frequently than other classes in the application
The wrong object causes more trouble than no object at all
Avdi Grimm
Duplication is far cheaper than the wrong abstraction
Sandi Metz
module UserService
def self.convert_subscription_request_to_user!(subscription_request)
end
def self.verify_registration_request!(registration)
end
def self.update_user!(update_request, scentregroup_id)
end
def self.update_user_from_registration!(registration, user)
end
end
Try REALLY hard only to have one public method.
def self.convert_subscription_request_to_user!(subscription_request)
activerecord::base.transaction do
subscription_request.update!(confirmed_at: time.zone.now)
user = user.find_by(email: subscription_request.email.downcase.squish)
operation = profile_operation_updated
if user.nil?
operation = profile_operation_created
user = create_user_from_subscription_request!(subscription_request)
end
create_interests!(subscription_request, user)
[user, operation]
end
end
class SubscriptionRequestToUserService
attr_accessor :subscription_request, :user
def initialize(subscription_request)
@subscription_request = subscription_request
end
def call
activerecord::base.transaction do
confirm_request!
user = get_user
operation = get_operation(user)
user ||= create_user_from_subscription_request!
SubscriptionRequestToInterest.new(user, subscription_request).call
[user, operation]
end
end
private
def confirm_request!
subscription_request.update!(confirmed_at: time.zone.now)
end
def get_user
@user ||= user.find_by(email: subscription_request.email.downcase.squish)
end
def get_operation(user)
user ? profile_operation_updated : profile_operation_created
end
end
class SubscriptionRequestToUserService
def self.call(subscription_request)
activerecord::base.transaction do
confirm_request!(subscription_request)
user = get_user(subscription_request)
operation = get_operation(user)
user ||= create_user_from_subscription_request!(subscription_request)
SubscriptionRequestToInterest.new(subscription_request).call
[user, operation]
end
end
private
def self.confirm_request!(subscription_request)
subscription_request.update!(confirmed_at: time.zone.now)
end
def self.get_user(subscription_request)
user.find_by(email: subscription_request.email.downcase.squish)
end
def self.get_operation(user)
user ? profile_operation_created : profile_operation_updated
end
end
Nil, "" and "John" a.k.a. the good, the bad and the ugly
user.first_name ||= profile.first_name
user.last_name ||= profile.last_name
if user.first_name.blank? && profile.first_name.present?
user.first_name = profile.first_name
end
if user.last_name.blank? && profile.last_name.present?
user.last_name = profile.last_name
end
class User < ActiveRecord::Base
# sadly, it is a callback
# nilify_blanks
# nilify_blanks :types => [:text]
# nilify_blanks :only => [:author, :title]
end
- You can’t easily change the method’s arguments.
- The method has three or more arguments.
- The method is complex due to number of collaborating parameters.
- The method requires large amounts of setup during isolated testing.
def self.create_phys_id!(user,
ident,
phys_provider,
ext_identifier,
minor_id,
extra_details = {})
def self.create_phys_id!(user,
ident:,
phys_provider:,
ext_identifier:,
minor_id:,
extra_details: {})
CreatePhysicalIdentifierService.call(params)
# or parameter object
- confirmations_controller.rb
- omniauth_callbacks_controller.rb
- passwords_controller.rb
- registrations_controller.rb
- sessions_controller.rb
- unlocks_controller.rb
get '/subscriptions', to: 'subscriptions#index'
get '/subscriptions/options', to: 'subscriptions#options'
get '/subscriptions/is_subscribed', to: 'subscriptions#is_subscribed'
put '/subscriptions', to: 'subscriptions#update'
delete '/subscriptions', to: 'subscriptions#destroy'
post '/subscriptions/resubscribe', to: 'subscriptions#resubscribe'
post '/unsubscribe', to: 'unsubscribes#create'
resource :subscriptions, only: %i(index update destroy)
get '/subscriptions/options', to: 'subscription_options#show'
get '/subscriptions/is_subscribed', to: 'check_subscription#index'
post '/subscriptions/resubscribe', to: 'resubscribe#create'
post '/unsubscribe', to: 'unsubscribes#create'
ActiveInteraction gives you a place to put your business logic. If ActiveModel deals with your nouns, then ActiveInteraction handles your verbs.
require 'active_interaction'
class Square < ActiveInteraction::Base
float :x
def execute
x**2
end
end
outcome = Square.run(x: 'two point one')
outcome.valid?
# => nil
outcome.errors.messages
# => {:x=>["is not a valid float"]}
outcome = Square.run(x: 2.1)
outcome.valid?
# => true
outcome.result
# => 4.41
def index
@accounts = ListAccounts.run!
end
class ListAccounts < ActiveInteraction::Base
def execute
Account.not_deleted.order(last_name: :asc, first_name: :asc)
end
end
interactions/
pubsub_in/
advam
skyfii
pubsub_out/
parking
registrations/
create_user
expire
verify
users/
find_by_physical_identifer
find_by_scentregroup_id
find_by_email
merge
80 Characters per line?
Timestamps
SubscriptionRequest
http://www.railspoint.com/brown_bag1
- flog, rubycritic, codeclimat?
- single responsibility methods?
- single responsibility services/classes?
- Nilify Blanks
- arity - 1-2 arguments + named attributes?
- REST - 1 additional method, otherwise new controller?
- ActiveInteraction
- 160 charackters per line?
- Timestamps
- BrownBag presentations