From c793684d4cabaeb0f2e577ff8a3a57752a94c4fd Mon Sep 17 00:00:00 2001 From: Josh Susser and Avdi Grimm Date: Thu, 16 May 2013 01:19:34 -0400 Subject: [PATCH 1/4] Replace exception used for flow control with idiomatic throw/catch. --- lib/pretty_text.rb | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/lib/pretty_text.rb b/lib/pretty_text.rb index 623adec9cd0..583e3cc3f96 100644 --- a/lib/pretty_text.rb +++ b/lib/pretty_text.rb @@ -235,8 +235,6 @@ module PrettyText class ExcerptParser < Nokogiri::XML::SAX::Document - class DoneException < StandardError; end - attr_reader :excerpt def initialize(length,options) @@ -249,10 +247,8 @@ module PrettyText def self.get_excerpt(html, length, options) me = self.new(length,options) parser = Nokogiri::HTML::SAX::Parser.new(me) - begin + catch(:done) do parser.parse(html) unless html.nil? - rescue DoneException - # we are done end me.excerpt end @@ -303,7 +299,7 @@ module PrettyText @excerpt << encode.call(string[0..length]) if truncate @excerpt << "…" @excerpt << "" if @in_a - raise DoneException.new + throw :done end @excerpt << encode.call(string) @current_length += string.length if count_it From d30330441ab5b086b85a1c546125fc0218265b63 Mon Sep 17 00:00:00 2001 From: Josh Susser and Avdi Grimm Date: Fri, 17 May 2013 14:06:58 -0400 Subject: [PATCH 2/4] Refactored conditional to an || to be more idiomatic. --- app/models/group.rb | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/app/models/group.rb b/app/models/group.rb index c96248ebd29..e16af207bf0 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -77,10 +77,7 @@ class Group < ActiveRecord::Base end def self.[](name) - unless g = lookup_group(name) - g = refresh_automatic_group!(name) - end - g + lookup_group(name) || refresh_automatic_group!(name) end def self.lookup_group(name) From 5659b667291940881da2b18ba03cfd0df5f93647 Mon Sep 17 00:00:00 2001 From: Josh Susser and Avdi Grimm Date: Fri, 17 May 2013 15:11:37 -0400 Subject: [PATCH 3/4] Refactor select().map() to use pluck. Remove a method already provided by ActiveRecord. --- app/models/category.rb | 2 +- app/models/group.rb | 6 +----- app/models/post_action.rb | 2 +- lib/post_destroyer.rb | 2 +- 4 files changed, 4 insertions(+), 8 deletions(-) diff --git a/app/models/category.rb b/app/models/category.rb index 6ce6d3afc12..6f079e606d1 100644 --- a/app/models/category.rb +++ b/app/models/category.rb @@ -112,7 +112,7 @@ class Category < ActiveRecord::Base def group_names=(names) # this line bothers me, destroying in AR can not seem to be queued, thinking of extending it category_groups.destroy_all unless new_record? - ids = Group.where(name: names.split(",")).select(:id).map(&:id) + ids = Group.where(name: names.split(",")).pluck(:id) ids.each do |id| category_groups.build(group_id: id) end diff --git a/app/models/group.rb b/app/models/group.rb index e16af207bf0..2d6a6bf058b 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -129,11 +129,7 @@ class Group < ActiveRecord::Base end def usernames - users.select("username").map(&:username).join(",") - end - - def user_ids - users.select('users.id').map(&:id) + users.pluck(:username).join(",") end def add(user) diff --git a/app/models/post_action.rb b/app/models/post_action.rb index ac3f687797d..df01b0f5b21 100644 --- a/app/models/post_action.rb +++ b/app/models/post_action.rb @@ -26,7 +26,7 @@ class PostAction < ActiveRecord::Base .count('DISTINCT posts.id') $redis.set('posts_flagged_count', posts_flagged_count) - user_ids = User.staff.select(:id).map {|u| u.id} + user_ids = User.staff.pluck(:id) MessageBus.publish('/flagged_counts', { total: posts_flagged_count }, { user_ids: user_ids }) end diff --git a/lib/post_destroyer.rb b/lib/post_destroyer.rb index 52c88ee3564..4675b487964 100644 --- a/lib/post_destroyer.rb +++ b/lib/post_destroyer.rb @@ -43,7 +43,7 @@ class PostDestroyer @post.update_flagged_posts_count # Remove any reply records that point to deleted posts - post_ids = PostReply.select(:post_id).where(reply_id: @post.id).map(&:post_id) + post_ids = PostReply.where(reply_id: @post.id).pluck(:post_id) PostReply.delete_all reply_id: @post.id if post_ids.present? From 2acc80d1926a243714dbe1d7da11add0d9e1cb8f Mon Sep 17 00:00:00 2001 From: Josh Susser and Avdi Grimm Date: Fri, 17 May 2013 15:44:35 -0400 Subject: [PATCH 4/4] Various idiomatic User refactorings. --- app/models/user.rb | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index d798d9793bd..08e419ddb00 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -66,16 +66,14 @@ class User < ActiveRecord::Base end def self.sanitize_username!(name) - name.gsub!(/^[^A-Za-z0-9]+|[^A-Za-z0-9_]+$/, "") - name.gsub!(/[^A-Za-z0-9_]+/, "_") + name.gsub!(/^[^[:alnum:]]+|\W+$/, "") + name.gsub!(/\W+/, "_") end - def self.pad_missing_chars_with_1s!(name) - missing_chars = User.username_length.begin - name.length - name << ('1' * missing_chars) if missing_chars > 0 - end def self.find_available_username_based_on(name) + sanitize_username!(name) + name = rightsize_username(name) i = 1 attempt = name until username_available?(attempt) @@ -100,14 +98,13 @@ class User < ActiveRecord::Base name = Regexp.last_match[2] if ['i', 'me'].include?(name) end - sanitize_username!(name) - pad_missing_chars_with_1s!(name) - - # Trim extra length - name = name[0..User.username_length.end-1] find_available_username_based_on(name) end + def self.rightsize_username(name) + name.ljust(username_length.begin, '1')[0,username_length.end] + end + def self.new_from_params(params) user = User.new user.name = params[:name]