2
\$\begingroup\$

I have a complex phone validation that does the following:

  1. First, check if either a home phone, mobile phone or work phone is provided (there will be 3 text fields in form and at least one type of phone number is required).

  2. Strip out any characters from the phone number that is not a digit except 'x' and '+', since those represent extensions and are valid.

  3. Make sure that the phones that were provided actually pass the phone number regex test.

And this is how I implemented the 3 steps above:

  VALID_PHONE_FORMAT = /\A(?:(?:\+?1\s*(?:[.-]\s*)?)?(?:\(\s*([2-9]1[02-9]|[2-9][02-8]1|[2-9][02-8][02-9])\s*\)|([2-9]1[02-9]|[2-9][02-8]1|[2-9][02-8][02-9]))\s*(?:[.-]\s*)?)?([2-9]1[02-9]|[2-9][02-9]1|[2-9][02-9]{2})\s*(?:[.-]\s*)?([0-9]{4})(?:\s*(?:#|x\.?|ext\.?|extension)\s*(\d+))?\z/

  validate :phone_provided
  before_validation :clean_phone_numbers
  validates_format_of :phone_home, with: VALID_PHONE_FORMAT, if: :home_is_filled?
  validates_format_of :phone_mobile, with: VALID_PHONE_FORMAT, if: :mobile_is_filled?
  validates_format_of :phone_work, with: VALID_PHONE_FORMAT, if: :work_is_filled?
  validates_format_of :phone_fax, with: VALID_PHONE_FORMAT, if: :fax_is_filled?

 private

  def clean_phone_numbers
    if phone_home.present?
      self[:phone_home] = strip_bad_characters :phone_home
    end
    if phone_mobile.present?
      self[:phone_mobile] = strip_bad_characters :phone_mobile
    end
    if phone_work.present?
      self[:phone_work] = strip_bad_characters :phone_work
    end
    if phone_fax.present?
      self[:phone_fax] = strip_bad_characters :phone_fax
    end
  end

  def strip_bad_characters(attr)
    send("#{attr}_before_type_cast").gsub(/[^\d+!x]/,'')
  end

  def phone_provided
    if phone_home.blank? && phone_mobile.blank? && phone_work.blank?
      errors.add(:base, "Must provide a phone number")
    end
  end

  def home_is_filled?
    !phone_home.blank?
  end

  def mobile_is_filled?
    !phone_mobile.blank?
  end

  def work_is_filled?
    !phone_work.blank?
  end

  def fax_is_filled?
    !phone_fax.blank?
  end

I particularly don't like how I am calling 4 different methods in the if condition for validates_format_of. I would like to only use one method and be able to tell which attribute to call blank? on. How can I refactor this?

\$\endgroup\$
2
\$\begingroup\$

Let's summarize the business rules:

  1. Characters other than digits, !, or x are stripped.
  2. If any of the four types of phone numbers is provided, it must be valid.
  3. At least one of { home, mobile, work } must be provided.

Requirement 3 is taken care of by validate :phone_provided, which is simple and fine.

The dissatisfaction comes from implementing Requirements 1 and 2.


The implementation of Requirement 1 can be simplified by making strip_bad_characters() tolerant of nil. You can also call #read_attribute_before_type_cast() to avoid interpolation.

def strip_bad_characters(attr)
  input = read_attribute_before_type_cast(attr)
  input.gsub(/[^\d+!x]/, '') if input
end

Also, clean_phone_numbers() can use a loop:

def clean_phone_numbers
  [:phone_home, :phone_mobile, :phone_work, :phone_fax].each do |attr|
    self[attr] = strip_bad_characters(attr)
  end
end

The solution for Requirement 2 is simple: wrap the entire regular expression in \A( )?\Z to make it optional. Then, you can get rid of all the …_is_filled? helpers.

\$\endgroup\$
1
\$\begingroup\$

You could do something like, using a little array magic

def clean_phone_numbers
  %w(phone_home phone_mobile phone_work phone_fax).each do |attribute|
    self[attribute.to_sym] = strip_bad_characters(attribute) if send(attribute).present?
  end
end

def phone_provided
  if [phone_home, phone_mobile, phone_work].all?(&:blank?)
    errors.add(:base, "Must provide a phone number")
  end
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.