Checking name E-mail and unit-test

Question

Please tell me what you think. Is my code bad?

Function checking

#-*- coding: utf-8 -*-
import re
import unittest

def email_check(mail): # E-mail as argument func

    compil = re.compile(r"""
        ^([-a-z0-9_]+
            ((?![.][.])|([.]))
            ((["]["])|(["][!:,]+["]))?
        ([-a-z0-9_]+)?)
        @
        [-a-z0-9_]+
        [.]
        ([-a-z0-9_]+
            ([.][-a-z0-9_]+)?
        )$
        """, re.X)

    result = compil.search(mail)

    if result:
        length = mail.split('@')
        if len(length[0]) <= 128 and len(length[1]) <= 256:
            print mail, 'correct' if not '-.' in length[1] and not '.-' in length[1] else 'Incorrect value'
        else:
            print 'Length string is not mathing'
    else:
        print 'Incorrect data'
    return result.group()

And unit-test

class TestCorrectEmail(unittest.TestCase):

    def setUp(self):
        self.mails = ['[email protected]', '[email protected]', '[email protected]', '01mail""[email protected]', '01mail"!,:"[email protected]', '[email protected]']

    def test_email_check(self):
        for i in self.mails:
            self.assertTrue(i == email_check(i), 'Incorrect value')

    def tearDown(self):
        self.mails = None

if __name__ == '__main__':
    unittest.main()

Answer 1

If you are compiling your regular expression there is no improvement if you do this inside your method, move your regular expression declaration outside the method so it will not be compile it again every time you call 'email_check'

In the line where you do length = mail.split('@'), I think the best name is parts. Just make sure you have some edge cases acknowledge.

Do the split just once, what about someone send "bill@@hotmail.com"? your code will mark this as correct.

local_part, domain = mail.split('@', 1)
if len(local_part) <= 128 and len(domain) <= 256:
     print 'Error'

Or even better add this rule to the regular expression, like ^[a-z]{3,128}$ where it reads any characters from a to z, at least three times found and no more than 128 times.

Maybe there are other things, but this are the first things that come to my mind.

Keep coding :)

Answer 2

If you are writing this code because you really need a mail validator, well, I suggest you to read this interesting post.

Instead, if you are writing code just because you like to do it, then this is my modest review.

1. Run PEP8/Pylint checkers:

Apparently, your code is not that "stylish" (you can see the PEP8 results here: http://pep8online.com/s/QbjC4Dx7)

Also, don't forget to read these two documents - as you have no docstrings in your code!

• PEP8: Style Guide for Python Code
• PEP257: Docstring Conventions

2. Don't mix return and print statements.

Raise exceptions and use loggers, when needed. Your function returns .group() anyway, either it's a valid or not valid email address.

Try to write code for these tests:

def test_valid_email_check(self):
  for i in ['[email protected]', '[email protected]', ...]:
    assert email_check(i) is True

def test_invalid_email_check(self):
  for i in ['[email protected]', ...]:
    assert email_check(i) is False