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.
-
\$\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\$Sᴀᴍ Onᴇᴌᴀ– Sᴀᴍ Onᴇᴌᴀ ♦2021年03月05日 19:53:26 +00:00Commented Mar 5, 2021 at 19:53
2 Answers 2
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}")
-
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\$Toby Speight– Toby Speight2021年03月05日 20:31:10 +00:00Commented Mar 5, 2021 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\$Toby Speight– Toby Speight2021年03月05日 20:33:04 +00:00Commented Mar 5, 2021 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– Andreas T2021年03月05日 20:53:45 +00:00Commented Mar 5, 2021 at 20:53
This works for [email protected] and also with toto@[email protected] which is an RFC compliant email address.
def splitMail(mail: str):
parts = mail.rsplit('@', 1)
user = parts[0]
domain = parts[1]
return user, domain
-
\$\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– mdfst132021年09月01日 14:39:50 +00:00Commented Sep 1, 2021 at 14:39
Explore related questions
See similar questions with these tags.