Skip to content

Instantly share code, notes, and snippets.

@yosriady
Last active November 17, 2015 08:19
Show Gist options
  • Save yosriady/c898cf34888903666713 to your computer and use it in GitHub Desktop.
Save yosriady/c898cf34888903666713 to your computer and use it in GitHub Desktop.

Some observations:

  • magicNumber=3 should be defined as a class constant with a proper name such as CERTIFIED_HOST_TRESHOLD, this way we can also skip the comment since the variable name is self-explanatory.
  • the Inquiry.find call and the if conditional expression should not be in the User class at all. It makes more sense to have a method within the Inquiry class that returns whther or not a host has the required number of inquiries.
  • Everytime we save, the before_save create_new_password callback is triggered and changes the user's password to a new password. I doubt this is intended behaviour. Do you mean before_create? I'm going to assume this example ignores resetting password use cases. If we only allow the password attribute to be set once on account creation, we could use read_attribute :password to prevent future tampering of the password attribute.
  • The raw SQL query is hard to read. Alternatively, we could do host.inquries.where(:status => [:status_1, :status_2]))
  • Using enums (e.g.: enum gender: %w( female male ) ), we can avoid writing our own statusLookup methods, instead we can call for example Conversation.statuses[:to_pay] which is generated by Rails. Less code means less maintenance and less bugs.
  • Using just return if self.certified is more idiomatic. Even better, we could use the conditional parameters for lifecycle callbacks such as :unless and :if.
  • The logic for password generation should be extracted to a separate utility class i.e. PasswordGenerator. The magic constants 8 and the password character set should be extracted to a class constant. Alternatively, we can simply use the SecureRandom.hex(8) method.
  • It's actually incorrect for us to certify the user within an after_save callback of the User class. For example, consider this case: The host has created more than 3 inquiries but have not saved the User object since. Instead, the validation should be done in the Inquiry class.

There may be more fixes/improvements I have missed, but these should help improve the overall code quality and maintainability.

class User < ActiveRecord::Base
has_many :inquiries
has_many :host_inquiries, :class_name => 'Inquiry', :foreign_key => 'host_id'
validates_presence_of :password
before_save :create_new_password
after_save :certify_if_needed
def certify_if_needed
return true if self.certified = true
# How many successful inquiries a host needs to be certified
magicNumber=3
inquiries = Inquiry.find(:all, :conditions=> ["host_id = ? AND status IN ( ? , ? )", self.id,Inquiry.statusLookup['STATUS_ROOMORAMA_TO_PAY'],
Inquiry.statusLookup['STATUS_PAYMENT_COMPLETE']], :limit => magicNumber)
if !inquiries.blank? && inquiries.length >= magicNumber
self.update_attribute(:certified, true)
end
return self.certified
end
def create_new_password
newpass = ''
validchars = 'abcdefghjkmnpqrstuvwxyzABCDEFGJKMNPQRSTUVWXYZ23456789'
8.times do
r = rand(validchars.length)
newpass << validchars[r..r]
end
self.password = newpass
end
end
class User < ActiveRecord::Base
has_many :inquiries
has_many :host_inquiries, :class_name => 'Inquiry', :foreign_key => 'host_id'
validates_presence_of :password
before_create :create_new_password
def create_new_password
self.password = SecureRandom.hex(8)
end
end
class Inquiry < ActiveRecord::Base
CERTIFIED_HOST_INQUIRY_MINIMUM = 3
enum status: %w( to_pay payment_complete )
after_save :certify_host
def certify_host
return if !host || host.certified?
exceed_inquiry_minimum = (host.inquiries.where(status: [Inquiry.statuses[:to_pay],Inquiry.statuses[:payment_complete]].length >= CERTIFIED_HOST_INQUIRY_MINIMUM)
if !host.inquiries.blank? && exceed_inquiry_minimum
host.update_attribute(:certified, true)
end
return host.certified
end
end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment