5
\$\begingroup\$

The purpose of this code is to ask the user to input an email address but it must contain "@" and ".co". The code works but it is too long so I would like to know if there is any way to shorten it using functions.

email_address = input("What is your email address? ")
while "@" not in email_address:
    email_address = input("Your email address must have '@' in it\nPlease write your email address again: ")
    if len(email_address) <= 6 :
        email_address = input("Your email address is too short\nPlease write your email address again: ")
    if "." not in email_address:
        email_address = input("Your email address must have '.' in it\nPlease write your email address again: ")
while "." not in email_address:
    email_address = input("Your email address must have '.' in it\nPlease write your email address again: ")
    if len(email_address) <= 6 :
        email_address = input("Your email address is too short\nPlease write your email address again: ")
    if "@" not in email_address:
        email_address = input("Your email address must have '@' in it\nPlease write your email address again: ")
\$\endgroup\$
  • \$\begingroup\$ a@b.de is a valid email address which is rejected by your code. Why? \$\endgroup\$ – Graipher Jan 19 '17 at 22:30
  • 2
    \$\begingroup\$ I suggest you have a look at this implementation and read the various comments and answers pointing at flaws and bugs. \$\endgroup\$ – 301_Moved_Permanently Jan 20 '17 at 7:27
  • \$\begingroup\$ hackernoon.com/… \$\endgroup\$ – BCdotWEB Feb 23 '17 at 14:47
4
\$\begingroup\$

Okay so as a general comment, validation like this is fine (well except it's discarding potentially valid addresses with the check for . and the minimum length, but I guess you know that), but in a real application you most likely want to just send an email and check if it was received, that also deals with a whole lot of other problems.

That said, of course this snippet can be shortened. I don't see why the two permutations of the loop are necessary in the first place.

Everything duplicated has the potential to be eliminated, but the result might be more or less readable and possibly longer (because there are only two cases here for example, so making the code more generic would only pay off with more cases).

What's the function going to be here? Checking if all the characters are in the email string, e.g. check_email_contains. What varies? Only the specific characters really. So pass that in and return the final address (without using globals) to be future proof.

Looks like this:

email_address = check_email_contains(input("What is your email address? "), "@.")

Lastly, the check function is very simple too:

def check_email_contains(email_address, characters, min_length=6):
    while True:
        for character in characters:
            if character not in email_address:
                email_address = input("Your email address must have '{}' in it\nPlease write your email address again: ".format(character))
                continue
        if len(email_address) <= min_length:
            email_address = input("Your email address is too short\nPlease write your email address again: ")
            continue
        return email_address

The loops in there are doing a bit too much, I'll leave that as an exercise for the reader to optimise it.

| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

For a series of validation checks, each of which has a corresponding error message or other response, I prefer a table-structured approach, rather than nested conditionals.

validations = (
  # Validation callable     # Error response
  (lambda s: '@' in s,      "must have '@' in it"),
  (lambda s: len(s) > 6,    "is too short"),
  (lambda s: '.' in s,      "must have '.' in it"),
)

def email_invalid(address):
  # type: str -> Union[str,None]
  """Check address against a series of rules which must all pass.

  Returns an error string on failure; None on success.
  """
  for valid, message in validations:
    if not valid(address):
      return message

Using this in your while loop is left as an exercise for the reader.

Note the name of the function is email_invalid, because it returns None on success and a non-empty string (which bool evaluates as True), so it works kind of as a double-negative. I have also chosen to make the validators themselves positive assertions for two reasons: it makes the lambdas more compact, so they fit better into the table and because it's easier to think about positive assertions [citation needed].

The benefits of this approach are that the validators are more prominent than when they're nested down in conditionals and they're easier to add, remove or disable by commenting-out. Since they can be programmatically extracted, tests can be written for the individual validators (which might not be necessary here, but applies generally). Validators that are more complicated than fit into a lambda can be regular functions (or even classes with a __call__ method).

Depending on the circumstances, it might be preferable to perform all the validations and return a list of error messages, so the user can fix them all at once. With this approach, it's a simple change to use a list comprehension instead of a for loop (and, like None, an empty list is also False).

The drawbacks of this approach are that lambda and using higher-order functions might be too advanced, depending on your (or your team's) skill or knowledge level; you have to be careful with polarity, both in deciding which to use where and in naming; and even if you know how to do this, it's often not until you've written it with conditionals that you realize you could have structured it like this, so it usually happens by refactoring.

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

Something else you may want to consider is some users may have numbers in their email address e.g. person123@hoster.com. The input function does not like having numbers in the input unless you have int() around the entire line of code. This will make the processing of the input data a bit better.

| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

I will suggest you to use regular expressions to match the wanted format.

Look the following function:

import re
def check_email_address(address):
  # Checks if the address match regular expression
  is_valid = re.search('^\w+@\w+.\w+$', address)
  # If there is a matching group
  if is_valid:
    return True
  else:
    print('It looks that provided mail is not in correct format. \n'
          'Please make sure that you have "@" and "." in your address \n'
          'and the length of your mail is at least 6 characters long')
    return False

The function will check whether the provided e-mail address is in (example@host.com) format.

Therefore if you call the function like this:

check_email_address('myexamplemail@examplehost.com')
>>True #Output is True

Otherwise it would print the error message and return False.

Furthermore if you write the validation like that, you can use it along with a while loop:

email_address = input('Please enter your e-mail address:')

while not check_email_address(email_address):
  # Keep prompting for email if not valid
  email_address = input('Please enter your e-mail address:')
else:
  # Do something with the valid e-mail address

The example regular expression matches "^\w+@\w+.\w+$" pattern, where:

"^" - start of the string

"\w" - matches any alphanumeric character

"+" - is quantifier (match one or more occurences)

"@" - matches the character "@" literally

"$" - end of the string

For more information about regular expression syntax, you can refer to here.

| improve this answer | |
\$\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.