Skip to main content
It turned out the `org` is optional for the `User` model
Source Link

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

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

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
Source Link

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