Slide 1

Slide 1 text

CLEAN CODE again Percy Lam

Slide 2

Slide 2 text

No content

Slide 3

Slide 3 text

No content

Slide 4

Slide 4 text

No content

Slide 5

Slide 5 text

OBJECTIVES Readable Simple Extendable Efficient

Slide 6

Slide 6 text

1 2 # artstack/app/models/user.rb 3 4 class User < ActiveRecord::Base 5 6 def self.search_users_ac(term) 7 User.joins(:profile) 8 .where("CONCAT(profiles.first_name, ' ', profiles.last_name) 9 LIKE ? OR users.username LIKE ?", '%'+term+'%', '%'+ 10 term+'%') 11 .limit(10) 12 end 13 14 end

Slide 7

Slide 7 text

NAMING • Pronounceable • Searchable • Avoid abbreviations

Slide 8

Slide 8 text

Searchable Names ok() book token stroke cookie smoking

Slide 9

Slide 9 text

1 # artstack/app/models/social_share.rb 2 3 class SocialShare 4 5 def self.tw_share_work_by_link(current_tw_user, work, 6 artwork_link) 7 if current_tw_user 8 message = "#{work.title} by #{work.artist} #{artwork_link} 9 @theArtStack #art" 10 current_tw_user.update(message) 11 end 12 end 13 14 end

Slide 10

Slide 10 text

Functions should… DO ONE THING!!

Slide 11

Slide 11 text

1 # artstack/app/controllers/api_controller.rb 2 3 class ApiController < ApplicationController 4 5 def register 6 if params[:fb_id] && params[:fb_access_token] && params[:fb_expires] 7 begin 8 user = User.find_by_facebook_uid(current_facebook_user.id) 9 if user 10 result = {:code => -2, :result => 11 'There exists an account associated to the same Facebook account. Please 12 tap Sign In instead.'} 13 14 if facebook_login(user)[:code] == 1 15 result[:user] = facebook_login 16 end 17 18 render :json => result 19 return 20 end 21 22 unless current_facebook_user && current_facebook_user.id == params[:fb_id].to_i 23 render :json => {:code => -2, :result => 'Account not associated with 24 Facebook yet. Please Sign In with password.'} 25 return 26 end 27 28 user = User.create(:show_welcome => false, :facebook_uid => params[:fb_id], : 29 signup_source => 'm') 30 user.before_connect(current_facebook_user) # simulate normal web facebook 31 connect flow, which to help create profile 32 33 begin 34 user.save! 35 rescue Exception => e 36 ExceptionLog.log(e, request) 37 render :json => {:code => -2, :result => 'Account not saved due to server 38 hiccups. Please try again later.'}

Slide 12

Slide 12 text

39 return 40 end 41 42 user.signup_follow(current_facebook_user) # simulate web user session creation, 43 auto follow users 44 user.profile.reload 45 46 add_device_token_to_user(params, user) 47 app_token = app_key ? UserApp.find_or_create_by_app_key_id_and_user_id(app_key. 48 id, user.id) : nil 49 app_token = app_token.try(:token) 50 51 render :json => { 52 :code => 1, 53 :result => 'Success', 54 :next_step_url => url_for(:controller => :settings, :action => :recommended, : 55 only_path => false, :app_token => app_token), 56 :first_name => user.profile.first_name, 57 :profile_url => user_profile_path(user, :only_path => false), 58 :sharing_url => url_for(:controller => :home, :action => :index, 59 :referrals => user.public_invitation_key, :only_path => false, :host => 60 ArtStack::DomainHost), 61 :app_token => app_token, 62 :id => user.id, 63 :avatar_url => user.profile.avatar_url(:thumb), 64 :follow_require_approval => user.follow_require_approval, 65 } 66 return 67 68 rescue => e 69 ExceptionLog.log(e, request) 70 render :json => {:code => -1, :result => 'Access token not correct'} 71 return 72 end 73 end 74 75 if params[:email] && params[:password] && params[:first_name] && params[:last_name] 76 # register by email and password 77 end 78 end

Slide 13

Slide 13 text

Function should… • be small • do one thing • reveal the intent by its name • use descriptive names

Slide 14

Slide 14 text

1 class FacebookRegisterHandler 2 3 def initialize(fb_user, params, request, controller) 4 @fb_user = fb_user 5 @params = params 6 @request = request 7 @controller = controller 8 9 begin 10 if validate_fb_user 11 unless check_if_user_already_exists 12 create_user 13 end 14 end 15 rescue => e 16 access_token_error_message 17 ExceptionLog.log(e, request) 18 end 19 end 20 21 private 22 23 def validate_fb_user 24 if @fb_user.valid? 25 true 26 else 27 fb_id_invalid_message 28 return false 29 end 30 end 31 32 def check_if_user_already_exists 33 @user = User.find_by_facebook_uid(@fb_user.id) 34 35 if @user 36 add_device_token_to_user 37 account_already_exists_message 38 end 39 end 40

Slide 15

Slide 15 text

41 def create_user 42 @user = User.create( 43 :show_welcome => false, 44 :facebook_uid => @fb_user.id, 45 :signup_source => 'm') 46 47 @user.before_connect(@fb_user) 48 49 begin 50 @user.save! 51 @user.signup_follow(@fb_user) 52 @user.profile.reload 53 add_device_token_to_user 54 success_message 55 56 rescue Exception => e 57 ExceptionLog.log(e, @request) 58 server_error_message 59 end 60 end 61 62 def success_message 63 @json = { 64 :code => 1, 65 :result => 'Success', 66 :next_step_url => h.url_for( 67 :controller => :settings, 68 :action => :recommended, 69 :only_path => false, 70 :app_token => app_token), 71 :first_name => @user.profile.first_name, 72 :profile_url => h.user_profile_path(@user, :only_path => false), 73 :sharing_url => h.url_for( 74 :controller => :home, 75 :action => :index, 76 :referrals => @user.public_invitation_key, 77 :only_path => false, 78 :host => ArtStack::DomainHost), 79 :app_token => app_token,

Slide 16

Slide 16 text

80 :id => @user.id, 81 :avatar_url => @user.profile.avatar_url(:thumb), 82 :follow_require_approval => @user.follow_require_approval, 83 } 84 end 85 86 def fb_id_invalid_message 87 @json = { 88 :code => -2, 89 :result => 'Account not associated with Facebook yet. Please Sign In with 90 password.' 91 } 92 end 93 94 def account_already_exists_message 95 @json = { 96 :code => -2, 97 :result => 'There exists an account associated to the same Facebook account. 98 Please tap Sign In instead.', 99 :user => { 100 :code => 1, 101 :result => 'Success', 102 :id => @user.id, 103 :first_name => @user.profile.first_name, 104 :profile_url => h.user_profile_path(@user, :only_path => false), 105 :sharing_url => h.url_for(:controller => :home, :action => :index, 106 :referrals => @user.public_invitation_key, :only_path => false, 107 :host => ArtStack::DomainHost), 108 :avatar_url => @user.profile.avatar_url(:thumb), 109 :app_token => app_token, 110 :follow_require_approval => @user.follow_require_approval, 111 } 112 } 113 end 114 115 end

Slide 17

Slide 17 text

Excessively long identifiers

Slide 18

Slide 18 text

Excessively long identifiers 1 # artstack/app/models/work.rb 2 3 class Work < ActiveRecord::Base 4 5 def update_artwork(params, opts = {}) 6 work_tags = []; 7 artist_tags = []; 8 updated_tags = [] 9 current_user = opts[:current_user] 10 11 # Get original tags of artwork 12 original_work_taggings = ActsAsTaggableOn::Tagging.select('tag_id, 13 context') 14 .where(:taggable_id => self.id, :taggable_type => :Work) 15 original_work_tagging_hash = original_work_taggings.index_by(&:tag_id) 16 original_work_tags = ActsAsTaggableOn::Tag.select('id, name') 17 .find(original_work_taggings.map(&:tag_id)) 18 .map { |ft| { :name => ft.name, :context => 19 original_work_tagging_hash[ft.id][:context] } } 20 .reject { |t| t[:context] == 'dimensions' } 21 22 # Get original tags of artist 23 original_artist_taggings = ActsAsTaggableOn::Tagging.select('tag_id, 24 context') 25 .where(:taggable_id => self.artist.id, :taggable_type => :Artist) 26 original_artist_tagging_hash = original_artist_taggings.index_by(&:tag_id) 27 original_artist_tags = ActsAsTaggableOn::Tag.select('id, name') 28 .find(original_artist_taggings.map(&:tag_id)) 29 .map { |ft| { :name => ft.name ,:context => 30 original_artist_tagging_hash[ft.id][:context] } }

Slide 19

Slide 19 text

31 32 # Set artwork type to the first medium matching ArtStack::ArtworkTypes 33 if params['medium_list'] && params["artwork_type_list"].blank? 34 artwork_types = ArtStack::ArtworkTypes.map(&:downcase) 35 mediums = params['medium_list'].split(';') 36 mediums.each do |medium| 37 if index = artwork_types.find_index { |type| type.singularize == 38 medium.strip.downcase.singularize } 39 params["artwork_type_list"] = ArtStack::ArtworkTypes[index] 40 break 41 end 42 end 43 end 44 45 # Add free tag 'Work in progress' 46 if params['work_in_progress'] == 'true' 47 params['tag_list'] += ';Work in progress' 48 end 49 50 # Construct array of tags input 51 ArtStack::TagTypes.each do |tag_type| 52 tag_list = tag_type.singularize + "_list" 53 if ArtStack::ArtistAndArtworkSharedTagTypes.include? tag_type 54 tag_list_param = (params[:artist_attributes] && params[: 55 artist_attributes][tag_list] && params[: 56 artist_attributes][tag_list]) || params[tag_list] 57 next if tag_list_param.blank? 58 tag_list_param = tag_list_param.split(/\;/) if tag_list_param.is_a? 59 String 60

Slide 20

Slide 20 text

• Function should be small • (Rule of thumb: < one screen size) • Code width < 80 ~ 100 characters

Slide 21

Slide 21 text

Long Parameter List

Slide 22

Slide 22 text

Long Parameter List 1 class ToPeopleJsonFromHashHandler 2 3 def user_followed_self 4 @result = a.to_users_feed_json_from_hash( 5 @activity[:followable_type], @activity[:followable], [@activity[:object] 6 ], @user) 7 8 header = I18n.t('application.feed.double_avatar.user_followed_self_html', 9 :user => a.link_to_user(@activity[:object], true, true, false, true), 10 :second_user => a.link_to_user(@activity[:followable], true, true, 11 false, true) 12 ) 13 14 header_short = I18n.t('application.feed.double_avatar. 15 user_followed_self_html', 16 :user => a.link_to_user(@activity[:object], true, false, false, true), 17 :second_user => a.link_to_user(@activity[:followable], true, false, 18 false, true) 19 ) 20 21 { 22 :header => header, 23 :header_short => header_short, 24 :first_avatar => @activity[:object], 25 :second_avatar => @activity[:followable] 26 } 27 end 28 29 end

Slide 23

Slide 23 text

Misplaced Responsibility

Slide 24

Slide 24 text

Misplaced Responsibility 1 # artstack/app/models/user.rb 2 3 class User < ActiveRecord::Base 4 5 # return utc offset which has the required local time in the 6 range [-11, 13] 7 8 def self.get_utc_offset_by_local(local) 9 now = Time.now.utc.hour 10 result = local - now 11 12 if result < -11 13 result + 24 14 elsif result > 13 15 result - 24 16 else 17 result 18 end 19 end 20 21 end

Slide 25

Slide 25 text

Feature Envy

Slide 26

Slide 26 text

1 class FollowableTag < ActiveRecord::Base 2 3 def self.update_exhibitions_from_mutual_art 4 from_date = (Date.today.at_beginning_of_month - 1.day).strftime('%Y/%m/%d') 5 to_date = (Date.today + 90.day).strftime('%Y/%m/%d') 6 mutual_art_url = "http://api.mutualart. 7 com/UpcomingExhibitions?username=mutualart@artstack. 8 com&password=2561A01O43Rbb5R&FromDate=#{from_date}&ToDate=#{ 9 to_date}" 10 response = JSON.load(open(mutual_art_url, :read_timeout => 300)) 11 12 exhibitions = response['Items'] || [] 13 exhibitions.each do |exh| 14 title = exh['Title'] 15 16 followable_tag = FollowableTag.followable_tag(title, 'exhibitions', true) 17 followable_tag.link_galleries_from_mutual_art 18 19 info = followable_tag.info 20 info.started_at = Date.parse(exh['StartDate']) if (exh['StartDate'] && !info. 21 started_at) 22 info.ended_at = Date.parse(exh['EndDate']) if (exh['EndDate'] && !info.ended_at) 23 info.institution = exh['Organization']['Name'] if (exh['Organization'].try('[]', 24 'Name') && !info.institution) 25 info.website = exh['Link'] if (exh['Link'] && !info.website) 26 27 if exh['Address'] && exh['Street'] 28 address = ([exh['Street']] + exh['Address'].split('/').reverse) 29 .collect{|str| str.gsub(/\s+/, " ").strip}.reject{|str| str.blank?}.uniq 30 .join(', ') 31 count = info.places.count 32 if count == 0 33 info.places << FollowableTagInfoPlace.create(:address => address) 34 elsif count == 1 35 place = info.places.first 36 if place.address != address 37 place.address = address 38 place.save 39 end 40 end

Slide 27

Slide 27 text

REMOVE ALL DEAD CODES

Slide 28

Slide 28 text

AVOID LARGE CLASSES

Slide 29

Slide 29 text

REPLACE MAGIC NUMBER WITH NAME CONSTANTS

Slide 30

Slide 30 text

0 20 40 60 80 100 120 140 160 180 200 Ideal Productivity vs time

Slide 31

Slide 31 text

END