Pull short id generation into separate class

app/models/comment.rb

@@ -7,7 +7,7 @@ class Comment < ActiveRecord::Base
     :class_name => "Comment"
   has_one :moderation,
     :class_name => "Moderation"
-  
+
   attr_accessible :comment, :moderation_reason
 
   attr_accessor :parent_comment_short_id, :current_vote, :previewing,
@@ -23,10 +23,10 @@ class Comment < ActiveRecord::Base
   define_index do
     indexes comment
     indexes user.username, :as => :author
-    
+
     has "(upvotes - downvotes)", :as => :score, :type => :integer,
       :sortable => true
-    
+
     has is_deleted
     has created_at
 
@@ -59,7 +59,7 @@ class Comment < ActiveRecord::Base
 
     nil
   end
-  
+
   def as_json(options = {})
     h = super(:only => [
       :short_id,
@@ -83,18 +83,7 @@ class Comment < ActiveRecord::Base
   end
 
   def assign_short_id_and_upvote
-    10.times do |try|
-      if try == 10
-        raise "too many hash collisions"
-      end
-
-      self.short_id = Utils.random_str(6).downcase
-
-      if !Comment.find_by_short_id(self.short_id)
-        break
-      end
-    end
-
+    self.short_id = ShortId.new(self.class).generate
     self.upvotes = 1
   end
 
@@ -228,7 +217,7 @@ class Comment < ActiveRecord::Base
 
     self.save(:validate => false)
     Comment.record_timestamps = true
-    
+
     self.story.update_comment_count!
   end
 
@@ -276,7 +265,7 @@ class Comment < ActiveRecord::Base
   def generated_markeddown_comment
     Markdowner.to_html(self.comment)
   end
-    
+
   def comment=(com)
     # TODO: remove remove_mb4 hack
     self[:comment] = com.to_s.rstrip.remove_mb4
@@ -374,7 +363,7 @@ class Comment < ActiveRecord::Base
       return false
     end
   end
-  
+
   def is_deletable_by_user?(user)
     if user && user.is_moderator?
       return true

app/models/invitation.rb

@@ -4,7 +4,7 @@ class Invitation < ActiveRecord::Base
   attr_accessible nil
 
   validate do
-    if !email.to_s.match(/\A[^@ ]+@[^ @]+\.[^ @]+\z/)
+    unless email.to_s.match(/\A[^@ ]+@[^ @]+\.[^ @]+\z/)
       errors.add(:email, "is not valid")
     end
   end

app/models/short_id.rb

@@ -0,0 +1,42 @@
+class ShortId
+  attr_accessor :klass, :generation_attempts
+
+  def initialize(klass)
+    self.klass               = klass
+    self.generation_attempts = 0
+  end
+
+  def generate
+    until (generated_id = candidate_id) && generated_id.valid? do
+      self.generation_attempts += 1
+      raise 'too many hash collisions' if generation_attempts == 10
+    end
+    generated_id.to_s
+  end
+
+  def candidate_id
+    CandidateId.new(klass)
+  end
+
+  private
+  class CandidateId
+    attr_accessor :klass, :id
+
+    def initialize(klass)
+      self.klass = klass
+      self.id    = generate_id
+    end
+
+    def to_s
+      id
+    end
+
+    def generate_id
+      Utils.random_str(6).downcase
+    end
+
+    def valid?
+      !klass.exists?(short_id: id)
+    end
+  end
+end

app/models/story.rb

@@ -22,7 +22,7 @@ class Story < ActiveRecord::Base
   before_create :assign_short_id
   before_save :log_moderation
   after_create :mark_submitter
-  
+
   define_index do
     indexes url
     indexes title
@@ -128,17 +128,7 @@ class Story < ActiveRecord::Base
   end
 
   def assign_short_id
-    10.times do |try|
-      if try == 10
-        raise "too many hash collisions"
-      end
-
-      self.short_id = Utils.random_str(6).downcase
-
-      if !Story.find_by_short_id(self.short_id)
-        break
-      end
-    end
+    self.short_id = ShortId.new(self.class).generate
   end
 
   def log_moderation
@@ -201,7 +191,7 @@ class Story < ActiveRecord::Base
   def comments_url
     "#{short_id_url}/#{self.title_as_url}"
   end
-  
+
   def short_id_url
     Rails.application.routes.url_helpers.root_url + "s/#{self.short_id}"
   end
@@ -262,7 +252,7 @@ class Story < ActiveRecord::Base
   def score
     upvotes - downvotes
   end
-  
+
   def vote_summary
     r_counts = {}
     Vote.where(:story_id => self.id, :comment_id => nil).each do |v|
@@ -309,7 +299,7 @@ class Story < ActiveRecord::Base
       end
     end
   end
-  
+
   def tagging_changes
     old_tags_a = self.taggings.reject{|tg| tg.new_record? }.map{|tg|
       tg.tag.tag }.join(" ")
@@ -367,7 +357,7 @@ class Story < ActiveRecord::Base
       return false
     end
   end
-  
+
   def is_undeletable_by_user?(user)
     if user && user.is_moderator?
       return true

spec/models/comment_spec.rb

@@ -0,0 +1,9 @@
+require "spec_helper"
+
+describe Comment do
+  it "should get a short id" do
+    c = Comment.make!(:comment => "hello")
+
+    c.short_id.should match(/^\A[a-zA-Z0-9]{1,10}\z/)
+  end
+end

spec/support/blueprints.rb

@@ -22,3 +22,9 @@ Story.blueprint do
   url { "http://example.com/#{sn}" }
   tags_a { [ "tag1", "tag2" ] }
 end
+
+Comment.blueprint do
+  user_id { User.make!.id }
+  story_id { Story.make!.id }
+  comment { "comment text #{sn}" }
+end