Validate if the user is associated with the organization then it must have any valid timezone

Question

def validate_time_zone
  if time_zone.present? && org.present? && !org.country.states.map(&:time_zone).uniq.include?(time_zone)
    errors.add(:time_zone, :invalid)
  end
end

I have a user model with the following relations:

• User can optionally belong to Org
• Org belongs to a Country
• A Country has some timezones (not unique as two states may have the same timezone).

So I validate that if the user is associated with an organization then it must have a valid timezone. Valid timezone means any time zone that comes under organization country. Ex Organization is ABC, Country is USA and Timezones are CET, Pacific, and Other US Time Zone. Then the user must use a timezone within the above three only.

I have filtered the condition but I am not sure breaking down will improve quality or further performance.

Can we used scope here too in country or organization or user module as it help in DRY not just validation.

Answer 1

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 :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

Answer 2

I would do org.country.states.where(time_zone: time_zone).empty? instead of not + map/pluck + include?, there's no need to get an array of time zones.

And I'd move the condition to a helper private method for readabilty.

def validate_time_zone
  errors.add(:time_zone, :invalid) if time_zone_not_in_country?
end

private
def time_zone_not_in_country?
  time_zone.present? and org.present? and org.country.states.where(time_zone: time_zone).empty?
end

You could even move that ugly org.country.states.... chain to a method on the Org model like incluedes_time_zone?(time_zone) (read about the Law of Demeter)

def time_zone_not_in_country?
  time_zone.present? and !org.try(:includes_time_zone?, time_zone) #using `try` to skip that `org.present?` call too
end

and on Org.rb

def includes_time_zone?(tz)
  country.states.where(time_zone: tz).any?
end