3
\$\begingroup\$

This is my first ever Python program and I wanted to get everyone's opinions on it. The goal of the program is to separate an email address into its username and domain name.

import sys
import getopt

argv = sys.argv[1:]

# -------Error Handling-Start----------
try:
    opts, args = getopt.getopt(argv, "l:s:h", ["list ", "single ", "help "])
except getopt.GetoptError as err:
    print(f"{err}\n""\n"
          "Usage: <Options> <Input> \n"
          "-l, --list        Specify a list of emails to be sliced \n"
          "-s, --single      Specify a single email to be sliced \n"
          "-h, --help        Show options")
    opts = []
# -------Error Handling-End------------

for opt, file_arg in opts:
    if opt in ['-h', '--help']:
        sys.exit("Usage: <Options> <Input> \n"
                 "-l, --list        Specify a list of emails to be sliced \n"
                 "-s, --single      Specify a single email to be sliced \n"
                 "-h, --help        Show options")
# If option -h is present, display MAN page

for opt, email_arg in opts:
    if opt in ['-s', '--single']:
        email = email_arg
        username = email[:email.index('@')]
        domain = email[email.index('@') + 1:]
        print(f"Your username: {username}")
        print(f"Your domain name: {domain}\n")
# If option -s is present, split input email into username and domain then print the output

for opt, file_arg in opts:
    if opt in ['-l', '--list']:
        file = file_arg

        email_file = open(file, "r")

        for string in email_file:
            username = string[:string.index('@')]
            domain = string[string.index('@') + 1:]
            print(f"Your username: {username}")
            print(f"Your domain name: {domain}")
# If option -l is present read file specified then loop through each line while splitting each into username and domain

if len(sys.argv) == 1:
    sys.exit("Usage: <Options> <Input> \n"
             "-l, --list        Specify a list of emails to be sliced \n"
             "-s, --single      Specify a single email to be sliced \n"
             "-h, --help        Show options")
# If only one argument is supplied, print MAN page.
\$\endgroup\$
1
  • \$\begingroup\$ Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Feel free to edit and give it a different title if there is something more appropriate. \$\endgroup\$ Mar 5 '21 at 19:53
2
\$\begingroup\$

The general style looks good. The biggest opportunity for improvement is your use of getopt. You should use the newer and easier argparse instead. Most of your code is used to parse and use the command line arguments. All of that can be done in three lines with argparse.

import argparse

arg_parser = argparse.ArgumentParser()
arg_parser.add_argument("-f", "--file", dest="file", default=False, action="store_true")
arg_parser.add_argument("input", type=str)
args = arg_parser.parse_args()

Instead of using --single or --list, I added a single flag --file (or -f), as the former choice is not only ambiguous (what happens if you specify neither or both?) as well as potentially misleading in their name. A help option with "-h" is introduced automatically.

"-f", "--file" are the flags you can use to specify this argument. dest=file means that the created object args wil have an attribute args.file that stores the resulting value. action="store_true" means that the flag has to be specified without a value (i.e. -f instead of -f True) to set it.

After that, you can access your inputs as a strings (because type=str) by args.input (because the first argument to add_argument was "input").

open of a file should always be paired with a close when you are done using it, as is also true for other I/O objects such as network connections. Instead of manually calling close, one should prefer to use Python's context managers with the with keyword, which automatically perform any cleanup.

if args.file:
    with open(args.input, "r") as file:
        emails = file.readlines()
else:
    emails = [args.input]

Your code to split the email into username and domain can be done with str.split, assuming that there is exactly one "@". If there are not "@" or more than one, you should probably notify the user that there is an error anyway. If you want to have actual "serious" code that goes into production in a live system, see Toby Speight's comment on pointers how to properly do that.

    if email.count("@") != 1:
        ...  # Notify the user
    username, domain = email.split("@")
    print(f"Your username: {username}")
    print(f"Your domain name: {domain}")

Finally, your use of sys.exit is incorrect. You should only use sys.exit(n) with an integer n. sys.exit(0) means "successful", anything else means "failed". Unless you have specific needs, sys.exit(1) is the default choice for failure.

To give user friendly error messages, the easiest way is to use print. You can show the messages as an error by specifying file=sys.stderr (as opposed to the default file=sys.stdout).

        print(f"{email} is not a valid email.", file=sys.stderr)
        sys.exit(1)

In total, this is what I'd propose your program could look like:

import sys
import argparse

arg_parser = argparse.ArgumentParser()
arg_parser.add_argument("-f", "--file", dest="file", default=False, action="store_true")
arg_parser.add_argument("input", type=str)
args = arg_parser.parse_args()

if args.file:
    with open(args.input, "r") as file:
        emails = file.readlines()
else:
    emails = [args.input]

for email in emails:
    if email.count("@") != 1:
        print(f"{email} is not a valid email.", file=sys.stderr)
        sys.exit(1)
    username, domain = email.split("@")
    print(f"Your username: {username}")
    print(f"Your domain name: {domain}")
\$\endgroup\$
3
  • 1
    \$\begingroup\$ I'd have to look up the relevant RFC (5322, these days?) to see if @ is allowed in the local part. If so, we'll want to find the last @ to identify the domain. There's far to much broken email-handling code around (I frequently encounter systems that think they are allowed to change the case of the local part!) that I wouldn't want to encourage more implementation by those who can't or won't read the RFC. \$\endgroup\$ Mar 5 '21 at 20:31
  • 1
    \$\begingroup\$ I think you misread the code - --single seems to be accepting arguments, and --list means read from file. I think they could helpfully be renamed, as your interpretation shows! \$\endgroup\$ Mar 5 '21 at 20:33
  • 1
    \$\begingroup\$ @TobySpeight You're completely right regarding the arguments. I'll update my answer to show the correct behavior. Regarding RFC5322, you are right as well. Although I think this should be more of a note "If you actually want a serious and safe system, don't try to implement an email parser yourself". Going into detail I feel would exceed the scope of a review on this level. \$\endgroup\$
    – Andreas T
    Mar 5 '21 at 20:53
-1
\$\begingroup\$

This works for toto@domain.com and also with toto@tutu@domain.com which is an RFC compliant email address.

def splitMail(mail: str):
    parts  = mail.rsplit('@', 1)
    user   = parts[0]
    domain = parts[1]
    
    return user, domain
\$\endgroup\$
1
  • \$\begingroup\$ This would be a stronger answer if it showed that the original code did not support @ symbols in the user ID part and provided a citation for the claim that they were RFC-compliant. It would be even stronger if there was a citation that showed that any email addresses actually use more than one @ symbol. You also might consider explaining the other choices that you made. E.g. why you assigned the two parts to variables rather than just returning them. \$\endgroup\$
    – mdfst13
    Sep 1 '21 at 14:39

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.