Check if email address contains one of many domains from a table, ignoring the subdomain

Question

I'm trying to validate email addresses as being from certain universities. I have a table, University, which is full of university domains. University email addresses often have different subdomains, and I want to be able to tolerate any subdomain under the domains in my table.

I think the following works, but is poorly written. Can anyone suggest ways of refactoring this, with explanations?

domain = self.email.split("@")[1]

domain_test = false

University.all.each do |d|
  if domain.include?(d.domain)
    domain_test = true
    break
  end
end

if domain_test = false
  errors.add(:email, "Sorry, no match found")
end

Answer 1

Looking at your current code first, I noticed the following:

• You should use email.split('@').last instead of an opaque numeric index (plus, you'll always get a string this way, whereas using [1] will return nil if the address doesn't contain an "@" to begin with)

• You should probably downcase the address you're trying to match (provided the university domains are similarly downcased, of course)

• You never need to loop, check a condition, and set a variable + break if that condition is true. The reason you don't need to that is because you have Enumerable#detect to do it for you.

• This looks like a Rails project, so it'd probably be faster to either let the database check for you, or only load the column you need, instead of loading every single University record.

For instance, to let the DB check it, you can do this

match = University.where("? LIKE CONCAT('%', domain)", self.email).any?
errors.add(:email, "Sorry, no match found") unless match

Since you're not actually loading any records doing this, I'd imagine it's pretty fast (and case-insensitive, so no need for downcasing)

As for matching things in Ruby, Nat's approach is the most straightforward one, I'd say.

I'd still do it a bit differently, though:

address = self.email.downcase

# only load the domain column values, since those are all we need here
match = University.pluck(:domain).detect { |domain| address.end_with?(domain) }

errors.add(:email, "Sorry, no match found") unless match

Of course, you should still check that the address is a valid email address in its entirety.

Answer 2

here's what I would do:

University.all.each do |uni|
  if email.end_with?(uni.domain)
    errors.add(:email, "Sorry, no match found")
    break
  end
end

it's not necessary to split the email address first because the end_with? method only considers at final substrings of the domain.

Answer 3

Build a regular expression from all your university domain, build it just once. If you domains are brunel.ac.uk bton.ac.uk cam.ac.uk you need to build the expression

/brunel\.ac\.uk$|bton\.ac\.uk$|cam\.ac\.uk$/

like so:

regular_expression = Regexp.new(
    University.all.map{|domain| Regexp.escape(domain)}.join("$|") + "$"
)

then match with this expression:

regular_expression.match(domain)