Some observations:
magicNumber=3should be defined as a class constant with a proper name such asCERTIFIED_HOST_TRESHOLD, this way we can also skip the comment since the variable name is self-explanatory.- the
Inquiry.findcall and the if conditional expression should not be in theUserclass at all. It makes more sense to have a method within theInquiryclass that returns whther or not a host has the required number of inquiries. - Everytime we save, the
before_save create_new_passwordcallback is triggered and changes the user's password to a new password. I doubt this is intended behaviour. Do you meanbefore_create? I'm going to assume this example ignores resetting password use cases. If we only allow thepasswordattribute to be set once on account creation, we could useread_attribute :passwordto prevent future tampering of thepasswordattribute. - 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 exampleConversation.statuses[:to_pay]which is generated by Rails. Less code means less maintenance and less bugs. - Using just
return if self.certifiedis more idiomatic. Even better, we could use the conditional parameters for lifecycle callbacks such as:unlessand:if. - The logic for password generation should be extracted to a separate utility class i.e.
PasswordGenerator. The magic constants8and the password character set should be extracted to a class constant. Alternatively, we can simply use theSecureRandom.hex(8)method. - It's actually incorrect for us to certify the user within an
after_savecallback 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 theInquiryclass.
There may be more fixes/improvements I have missed, but these should help improve the overall code quality and maintainability.