5
\$\begingroup\$

To learn about classes, I chose to create a little something that checks if you have included the following:

  • @ sign
  • Provider (e.g. 'hotmail')
  • Domain extension (e.g. '.com')
  • It also checks if you entered spaces in the address and gave the option to keep them or get rid of them.

I would really like some feedback on what I should improve on. If you have any questions about why I did what I did at a specific part of the code, ask away!

class email_checker:

    def __init__(self, email_input):
        self.email_input = email_input
        self.done = False
        self.fixed_domain = False
        self.redo = True
        self.answer = ''
        self.domain_extension = ''
        self.providers = ['hotmail', 'gmail', 'yahoo', 'outlook']
        self.de = ['com', 'org', 'net', 'edu', 'gov']

    def enter_email(self):
        self.email_input = input('Enter your email \n>')
        print('You entered: ' + self.email_input)
        self.provider_check()

    def provider_check(self):
        for i in self.providers:
            if i in self.email_input:
                self.at_check()
                return # I use this to exit this method when it returns from
                       # self.de_check()
                       # Making sure that it doesn't run the
                       # self.enter_email() method again.
        print('Please include which provider you\'re using. E.g. outlook')
        self.enter_email()


    def at_check(self):
        if not '@' in self.email_input:
            print('----\nYou missed the \'@\' sign! Let me fix that for you.')

            for i in self.providers:
                if i in self.email_input:
                    self.email_input = self.email_input.replace(i, '@' + i)
                    self.space_check()

    def space_check(self):
        if ' ' in self.email_input:
            self.fixed_email = self.email_input.replace(' ', '')
            print('----\nPlease choose:\n1: %s\n2: %s' % (self.fixed_email,
                                                          self.email_input))

            while self.done == False:
                self.answer = input('>').upper()
                if self.answer == '1'.upper():
                    self.email_input = self.fixed_email
                    self.done = True
                    self.de_check()
                elif self.answer == '2'.upper():
                    self.done = True
                    self.de_check()
                else:
                    print('Enter \'1\' or \'2\'')
                    self.done = False
        else:
            self.de_check()

    def de_check(self):
        # I tried to use a for loop to loop through the self.de list but
        # then the print would print five times. So I went with this route.
        if not 'com' in self.email_input and not 'org' in self.email_input \
        and not 'net' in self.email_input and not 'edu' in self.email_input \
        and not 'gov' in self.email_input:
            print('----\nYou forgot your domain extension! Choose one here: \
\n.com\n.org\n.net\n.edu\n.gov')

            while self.fixed_domain == False:
                self.domain_extension = input('>')
                if '.com' == self.domain_extension or '.org' == self.domain_extension \
                or  '.net' == self.domain_extension or '.edu' == self.domain_extension \
                or '.gov' == self.domain_extension:
                    self.email_input = self.email_input + self.domain_extension.lower()
                    self.fixed_domain = True
                else:
                    print('Enter one of the above. Don\'t forget the \'.\'!')

        else:
            pass

        if not '.' in self.email_input:
            print('----\nYou missed the \'.\' before your domain \
extension! Let me fix that for you.')

            for i in self.de:
                if i in self.email_input:
                    self.email_input = self.email_input.replace(i, '.' + i)

if __name__ == '__main__':
    email_input = ''
    email_checker = email_checker(email_input)
    email_checker.enter_email()
    print('Here is your email: ' + email_checker.email_input)

Test run:

Enter your email 
>>>example
You entered: example
Please include which provider you're using. E.g. outlook
Enter your email 
>>>example hotmail
You entered: example hotmail
----
You missed the '@' sign! Let me fix that for you.
----
Please choose:
1: example@hotmail
2: example @hotmail
>>>1
----
You forgot your domain extension! Choose one here: 
.com
.org
.net
.edu
.gov
>>>.com
Here is your email: example@hotmail.com
\$\endgroup\$
  • 1
    \$\begingroup\$ You have done a good work! but why didn't you use regular expressions? \$\endgroup\$ – Mahmood Muhammad Nageeb Nov 18 '15 at 21:24
9
\$\begingroup\$

My first comment would probably be that you shouldn't try to validate emails beyond checking that they have at least one character before an @ sign, one . after the @ sign, and at least one character after the .. The reason for this is that validating emails is hard (those are each separate links, fwiw). In general,

the only valid email address is one you can send email to

No matter how hard you try to validate that the email is valid in terms of format, it will never check for nonsense email addresses ("asdf@asd.com") or typoes ("my_anme@gmail.com"). So practically speaking, email validation is generally not worth a whole lot unless it is obviously wrong ("asdf").

That being said, I'll critique your code, specifically.

Naming

According to PEP8 the de-facto* code quality standard for Python, class names should be in PascalCasing, so you should rename the class EmailChecker.

Also, you could use better names in a few places. For example,

for i in self.providers:
    if i in self.email_input:
        self.at_check()

could be greatly improved if i were replaced by something like provider_name.

Documentation

You should include docstrings for all public classes, methods, functions, and modules. This will help you use your code later, and help others as well.

Constants and class-level attributes

Right now you have these values

self.providers = ['hotmail', 'gmail', 'yahoo', 'outlook']
self.de = ['com', 'org', 'net', 'edu', 'gov']

as instance variables, and you set them individually for each instance of your checker. Instead, you could reduce repetition and make them class-level attributes:

class EmailChecker:
    providers = ['hotmail', 'gmail', 'yahoo', 'outlook']
    de = ['com', 'org', 'net', 'edu', 'gov']

and then each instance will use the same lists. Speaking of, ...

Unnecessary constraints

Right now your providers and de are far from comprehensive - what if my email isn't in any of those, but is still valid? You're better off validating that the provider and domain extension look reasonable. If you're interested, this link shows an (incomplete) list of top-level domain extensions.

Concatenating strings

Instead of adding strings as "string" + input("some string"), use string.join or string.format. They are more efficient, and much easier to read.

Readability

This isn't readable as is

if not 'com' in self.email_input and not 'org' in self.email_input \
and not 'net' in self.email_input and not 'edu' in self.email_input \
and not 'gov' in self.email_input:

at the very least indent the 2nd and 3rd lines. Additionally, it is generally preferred to use parentheses to have multi-line statements instead of the \ character. Even better, however, would be a generator expression

if not any(domain_extension in self.email_input for domain_extension in self.de):

I have some more critiques but I'm out of time - if I get a chance I'll post more later.

\$\endgroup\$
  • \$\begingroup\$ This was just a small practice project I decided to do, I would never use it in something that would actually be used. Thanks for the critique, it gave me a good insight on how to improve my general coding. \$\endgroup\$ – SamCode Nov 13 '15 at 11:13
  • 1
    \$\begingroup\$ @SamCode, Another point is that when you do stuff like not com in self.email_input you don't say where it is within the string. In other words, the above test would happily accept com.hotmail@myusername... \$\endgroup\$ – holroy Nov 15 '15 at 2:01
  • 1
    \$\begingroup\$ @SamCode It's helpful with practice projects to say that they're not for real use, so people know that from the start. \$\endgroup\$ – SuperBiasedMan Nov 16 '15 at 10:03
1
\$\begingroup\$

First of all I see a lot of stuff defined in __init__, but you don't need to. answer and domain_extension are actually set when they're needed anyway so there's no need to create them as empty for init. And redo is never even used, so take that out. You also have email_input as a parameter but then in your own sample usage you pass an empty string. You should have it default to an empty string, meaning that people can opt to not enter it directly if they don't want.

class EmailChecker:

    def __init__(self, email_input=''):

check = EmailChecker("email@gmail.com")
check = EmailChecker()

But more importantly, the flow of your code is confusing and hard to follow. When email is entered it's automatically validated by running through provider_check, then if that test is passed it calls at_check, which in turn calls space_check. It seems like you were just chaining them as you went through, but this is not an intuitive system. All the functions should be separated, not chained. It would be better to have a validate function that calls all the separate tests instead.

Besides that, you could make it better and address a comment holroy posted.

when you do stuff like not com in self.email_input you don't say where it is within the string. In other words, the above test would happily accept com.hotmail@myusername

This illustrates an issue with just detecting all the valid strings. Instead, you should treat it intelligently. There should be @ and . symbols. So start with that. Have a function that first checks that these are contained in the correct order. Then you could use these detected characters to extract out the email's domain and the provider. Once you have those removed, you can validate them against the list of domains and providers you have. This is a much more modular process but it takes logical steps to ensure the basic syntax is accurate before moving on to test the domains.

Some nitpicks about printing. If you need to use quotation marks, don't escape them with backslash, just use the opposite kind of quotation marks to allow them.

print("Enter '1' or '2'")

You also print all the domain extensions at one point, but they're hardcoded! What happens when you try to add one of the many extensions you're missing? Instead, print them all using the actual domain_extension parameter. str.join makes it easier too:

print('----\nYou forgot your domain extension! Choose one here:')
print('\n'.join(self.domain_extension))

Which would return:

----
You forgot your domain extension! Choose one here:
.com
.org
.net
.edu
.gov
\$\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.