Refactoring complex phone validations

Question

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?

Answer 1

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.

Answer 2

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