I have a complex phone validation that does the following:
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).
Strip out any characters from the phone number that is not a digit except 'x' and '+', since those represent extensions and are valid.
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?