- Time: 20-30 min
- Level: Beginner/Intermediate
- Code: GitHub
Before I started writing this post I searched for other people looking at code code from this perspective - found one How deep is your code?
I needed an example of complicated Service Object and found it in one of the Discourse source files. I’m not criticizing this code by any mean and there is a great example of what I’m going to try to achieve in the same repository
I personally prefer small classes, short, single purpose methods over the one big piece of code. Having said that I have to admit that this approach has a significant downside - methods are short quite often because internally they call other methods, which call other methods, which call other methods… So you have to go down the rabbit hole before you figure out what method actually does, sometimes you loose the big picture in the meantime.
I don’t want to do this, I don’t want to look at 5-10 private methods to figure out what are the implications of calling a public method, I want public method to be able to tell me the story right away, the very first time I see it - maybe there is a way?
Once I saw a complexity metric described by Sandi Metz in her talk All the Little Things - the Squint Test.While this metric was used to measure the complexity of the nested conditionals, we could think about calling methods in a similar way. If a method calls other method let’s indent it as if it was a nested if:
def parent
child_level_two
# some code
end
def child_level_two
# some code
child_level_three
end
def child_level_three
# some code
child_level_four
# some code
end
now let’s write down all involved methods and indent each nested call
def parent
child_level_two
child_level_three
child_level_two
child_level_three
child_level_three
child_level_two
child_level_two
child_level_three
child_level_three
child_level_four
child_level_two
child_level_two
child_level_three
child_level_four
child_level_four
end
we got similar figure with changes in shape, the bigger is the shape change - the more nesting we have.
STEP #1
Code changes in this step can be found in the corresponding commit
Service object before refactoring, I’ve extracted external dependencies into dependencies.rb
and added placeholders for required methods
# user_updater.rb
require 'active_support'
require 'active_support/core_ext/object/blank'
require 'active_support/core_ext/time/calculations'
require 'active_model'
require_relative 'dependencies'
class UserUpdater
delegate :change_post_owner, to: :guardian
def update(attributes = {})
save_options = false
saved = nil
old_user_name = user.name.present? ? user.name : ""
user_profile = user.user_profile
user_profile.location = attributes.fetch(:location) { user_profile.location }
user_profile.dismissed_banner_key = attributes[:dismissed_banner_key] if attributes[:dismissed_banner_key].present?
user_profile.website = format_url(attributes.fetch(:website) { user_profile.website })
user_profile.profile_background = attributes.fetch(:profile_background) { user_profile.profile_background }
user_profile.card_background = attributes.fetch(:card_background) { user_profile.card_background }
if SiteSetting.enable_sso && !SiteSetting.sso_overrides_bio
user_profile.bio_raw = attributes.fetch(:bio_raw) { user_profile.bio_raw }
end
user.name = attributes.fetch(:name) { user.name }
user.locale = attributes.fetch(:locale) { user.locale }
user.date_of_birth = attributes.fetch(:date_of_birth) { user.date_of_birth }
if can_grant_title?(user)
user.title = attributes.fetch(:title) { user.title }
end
# special handling for theme_key cause we need to bump a sequence number
if attributes.key?(:theme_key) && user.user_option.theme_key != attributes[:theme_key]
user.user_option.theme_key_seq += 1
end
OPTION_ATTR.each do |attribute|
if attributes.key?(attribute)
save_options = true
if [true, false].include?(user.user_option.send(attribute))
val = attributes[attribute].to_s == 'true'
user.user_option.send("#{attribute}=", val)
else
user.user_option.send("#{attribute}=", attributes[attribute])
end
end
end
# automatically disable digests when mailing_list_mode is enabled
user.user_option.email_digests = false if user.user_option.mailing_list_mode
fields = attributes[:custom_fields]
if fields.present?
user.custom_fields = user.custom_fields.merge(fields)
end
User.transaction do
if user.user_option.save && user_profile.save && user.save
StaffActionLogger.new(@actor).log_name_change(
user.id,
old_user_name,
attributes.fetch(:name) { '' }
)
saved = true
end
end
saved
end
private
attr_reader :user, :guardian
def initialize(actor, user, guardian = Guardian.new(actor))
@user = user
@guardian = guardian
@actor = actor
end
def format_url(website)
return nil if website.blank?
website =~ /^http/ ? website : "http://#{website}"
end
end
STEP #2
This step was divided into few smaller chunks:
- #2.1 Refactor
UserUpdater#update
method (commit) - #2.2 Refactor methods that were extracted in 2.1
I’ll skip details here because those are quite straight forward extractions, they were done to simplify UserUpdater#update
method, make it small and easy to understand.
# user_updater.rb
class UserUpdater
delegate :change_post_owner, to: :guardian
def update(attributes = {})
old_user_name = user.name.present? ? user.name : ""
update_user_profile( user.user_profile, attributes )
update_user( user, attributes )
update_user_option( user.user_option, attributes )
save_user_data(user, old_user_name, attributes)
end
private
# ...
However I believe that too much information was hidden with this extraction. From the method definition we no longer see that:
- There is a transaction
- Some parts are updated only if particular conditions are met
- We use constant to determine what to update
- It’s not obvious that we return (and depend on the returned value somewhere)
true
/false
depending on the result of the transaction
In addition to hiding information, I strongly dislike how new UserUpdater#update_user_profile
method looks - it’s a private method that consists of only other private methods, and once again - no conditionals inside
# user_updater.rb
def update_user_profile( user_profile, attributes )
update_geo_data( user_profile, attributes )
update_web_data( user_profile, attributes )
update_background_data( user_profile, attributes )
update_bio_raw( user_profile, attributes )
end
I believe that methods like this only introduce additional level of complexity and have no right to exist
STEP #3
Code changes in this step can be found in the corresponding commit
In this step we drop private methods that were (almost) only wrappers around other private methods and change remaining method names to reflect the intention better
# user_updater.rb
class UserUpdater
delegate :change_post_owner, to: :guardian
delegate :log_user_name_change, to: :StaffActionLogger
def update(attributes = {})
old_user_name = user.name.present? ? user.name : ""
user_profile = user.user_profile
user_option = user.user_option
set_user_profile_geo_data( user_profile, attributes )
set_user_profile_web_data( user_profile, attributes )
set_user_profile_background_data( user_profile, attributes )
set_user_profile_bio_raw( user_profile, attributes )
set_user_bio(user, attributes, update_title: can_grant_title?(user) )
user.custom_fields = user.custom_fields.merge( attributes.fetch( :custom_fields, {} ) )
set_user_option_theme_key(user_option, attributes)
OPTION_ATTR.each { |attribute| set_user_option_single_attribute(user_option, attributes, attribute) }
# automatically disable digests when mailing_list_mode is enabled
user_option.email_digests = false if user_option.mailing_list_mode
return false unless User.transaction { user.user_option.save && user.user_profile.save && user.save }
log_user_name_change( user.id, old_user_name, attributes.fetch(:name) { '' } )
return true
end
the UserUpdater#update
has become almost three times bigger compared to the previous version however it now also tells us a better story.
It’s not perfect (and will never be) but we will make one more step to improve it and compare with the shorter version from Step #2 afterwards.
STEP #4
Code changes in this step can be found in the corresponding commit
In this step we will do final polishing and compare results
# user_updater.rb
# Short version
class UserUpdater
# ...
def update(attributes = {})
old_user_name = user.name.present? ? user.name : ""
update_user_profile( user.user_profile, attributes )
update_user( user, attributes )
update_user_option( user.user_option, attributes )
save_user_data(user, old_user_name, attributes)
end
# Two levels deep version
class UserUpdater
# ...
def update(attributes = {})
@attributes = attributes
old_user_name = user.name.present? ? user.name : ""
set_user_profile_geo_data
set_user_profile_web_data
set_user_profile_background_data
set_user_profile_bio_raw if should_update_user_profile_bio_raw?
set_user_bio( update_title: can_grant_title?(user) )
user.custom_fields = user.custom_fields.merge( attributes.fetch( :custom_fields, {} ) )
set_user_option_theme_key if should_update_user_option_theme_key?
OPTION_ATTR.each do |attribute|
set_user_option_single_attribute( attribute )
end
# automatically disable digests when mailing_list_mode is enabled
user_option.email_digests = false if user_option.mailing_list_mode
return false unless User.transaction { save_user_and_related_entities }
log_user_name_change( user.id, old_user_name, attributes.fetch(:name) { '' } )
return true
end
I believe longer version has the following benefits:
- Code tells great story with lots of implementation details
- It follows Open/Closed principle
- You're always at most one step away from the full private method definition
Summary
In order to be two levels deep code has to follow the following rules:
- Public methods can’t call other public methods of the same class
- Private/protected methods can’t call other private/protected methods
I have two more rules of thumb
- Avoid conditionals in private methods
- Conditionals have to reflect on private methods rather than on boolean expressions
Code:
Food for thought
- These rules are quite easy to follow in Service Objects because usually they describe some process/algorithm - where else they can be applied?
- How about delegated methods, can we threat them as private when measuring?
- How about accessor methods, can we threat them as private when measuring?
Top comments (0)