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

Clean code again

Clean code again

975ff6c8166f6f32889f004a904ffa39?s=128

Oursky Limited

July 03, 2015
Tweet

More Decks by Oursky Limited

Other Decks in Programming

Transcript

  1. CLEAN CODE again Percy Lam

  2. None
  3. None
  4. None
  5. OBJECTIVES Readable Simple Extendable Efficient

  6. 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
  7. NAMING • Pronounceable • Searchable • Avoid abbreviations

  8. Searchable Names ok() book token stroke cookie smoking

  9. 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
  10. Functions should… DO ONE THING!!

  11. 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.'}
  12. 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
  13. Function should… • be small • do one thing •

    reveal the intent by its name • use descriptive names
  14. 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
  15. 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,
  16. 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
  17. Excessively long identifiers

  18. 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] } }
  19. 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
  20. • Function should be small • (Rule of thumb: <

    one screen size) • Code width < 80 ~ 100 characters
  21. Long Parameter List

  22. 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
  23. Misplaced Responsibility

  24. 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
  25. Feature Envy

  26. 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
  27. REMOVE ALL DEAD CODES

  28. AVOID LARGE CLASSES

  29. REPLACE MAGIC NUMBER WITH NAME CONSTANTS

  30. 0 20 40 60 80 100 120 140 160 180

    200 Ideal Productivity vs time
  31. END