In case if you wanna improve only the validate_time_zone
method:
Performance wise I don't see anything sinister in your implementation.
I assume the states
table has a direct time_zone
field. If so, then we can use the pluck
method to avoid instantiation ActiveRecord objects.
Array.uniq
returns a new array by removing duplicate values. The idea is nobel but it means it will use more memory and will loop over the time zones array which will happen anyway in case of include?
On the other hand nowadays in software development the real cost is more in maintenance rather than in CPU cycles.
With this in mind, how about the following refactor?
class User
validates_presence_of :org, :time_zone
validate :validate_time_zone
def validate_time_zone
# using guard clause and let the built in validation do the presence validation
return unless org.present? && time_zone.present?
# A temp variable is a code smell, but here we have an underlying concept
# that is better to be named
allowed_time_zones = org.country.states.pluck(:time_zone)
# tip: you can use the .exclude? API if you don't want to a use guard clause
return if allowed_time_zones.include?(time_zone)
errors.add(:time_zone, :invalid)
end
end