1
\$\begingroup\$
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.

\$\endgroup\$
  • \$\begingroup\$ (Welcome to Code Review.) You use Country at least two times, just to be followed by organization county: is the latter a typing error? \$\endgroup\$ – greybeard May 2 at 4:41
3
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ What if organization may it might not be present. \$\endgroup\$ – Dinesh Saini May 2 at 3:11
  • \$\begingroup\$ You mean, the organisation is optional for users? \$\endgroup\$ – Peter Toth - Toma May 2 at 9:35
  • \$\begingroup\$ Yes its optional in osme scenario \$\endgroup\$ – Dinesh Saini May 2 at 12:07
  • \$\begingroup\$ Well, you forget to mention it in the original description, I edited it. You might wanna disclose the business logic for such a case when you wanna validate the time_zone of a user who doesn't belong to any organisation. \$\endgroup\$ – Peter Toth - Toma May 2 at 13:04
  • \$\begingroup\$ Agree. But is not there any way we can set some scope in country or user or organization. That could be rich data model. \$\endgroup\$ – Dinesh Saini May 2 at 18:31
0
\$\begingroup\$

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
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.