7
\$\begingroup\$

I've written this email program in Python, and I would like to get some feedback on it. (i.e. closing the server twice after the raw input, etc.)

import console
import socket
import smtplib
from getpass import getpass

user = raw_input('What is your name? ').title()
host_name = socket.gethostname()
print '\nThis program is being run by %s on machine name %s' % (user, host_name)

# here we collect the login information for the email 
# we also collect the email provider to automatically change the server
email_provider = raw_input('Gmail, AOL, Yahoo! or Comcast? ').title() 
email_user = raw_input('Type in your full email username. ') # collect the user name
email_pwd = getpass('Type in your email password. ') # collect the users password

if email_provider in ('Gmail', 'Google'):
    smtpserver = smtplib.SMTP("smtp.gmail.com",587)
if email_provider in ('Aol', 'AOL'):
    smtpserver = smtplib.SMTP("smtp.aol.com",587)
if email_provider in ('Yahoo', 'Yahoo!'):
    smtpserver = smtplib.SMTP("smtp.mail.yahoo.com",587)
if email_provider in ('Comcast'): 
    smtpserver = smtplib.SMTP("smtp.comcast.net",587)

smtpserver.ehlo()
smtpserver.starttls()
smtpserver.ehlo
smtpserver.login(email_user, email_pwd)


def main():
    run_again = 'y'
    while run_again == 'y':
        # here we are collecting the Sendto email address
        # we save the input to a variable named sendto
        sendto = raw_input('Email address to send message to: ')
        to = sendto
        CC = sendto

        subj = raw_input('Subject: ')

        header = 'To: ' + to + '\n' + 'From: ' + email_user + '\n' + 'Subject: ' + subj +'\n'
        print '\nMessage Details:' 
        print (header)

        assignment = raw_input('Message: ')

        msg = header + assignment + '\n'
        smtpserver.sendmail(email_user, to, msg)
        console.hud_alert('Your message has been sent!', 'success', 2.1)
        run_again = raw_input('Would you like to send another message? y or n')
        if run_again == 'n': smtpserver.close()
        else: smtpserver.close()

main()
\$\endgroup\$
1
  • \$\begingroup\$ if ...: smtpserver.close() else: smtpserver.close() -> smtpserver.close() \$\endgroup\$
    – belkka
    Commented Feb 17, 2019 at 14:43

2 Answers 2

5
+50
\$\begingroup\$
  • I would suggest putting all the code in main() except for the imports. Then at the bottom, do

    if __name__ == '__main__':
        main(sys.argv)
    

    The advantage of structuring your file this way is that you can run the python interpreter and import your code and play with it. If you break it up into multiple routines, you can run each routine by itself to be sure it does the right thing. As it is now, if you try to import the file, half the code will be run immediately and then main() gets run unconditionally at the bottom, so you can't control it easily in the python interpreter.

  • To translate email_provider to smtp_server, you could do

    host = {'GMAIL': "smtp.gmail.com",
            'GOOGLE': "smtp.gmail.com",
            'AOL': "smtp.aol.com",
            'YAHOO': "smtp.mail.yahoo.com",
            'COMCAST': "smtp.comcast.net"}
    
    smtpserver = smtplib.SMTP(host[email_provider.strip('!').upper()], 587)
    
  • And you might want to handle the case where the user types an unknown email provider.

    try:
        smtpserver = smtplib.SMTP(
              host[email_provider.strip('!').upper()], 587)
    except KeyError:
        print("Unknown email provider '%s'" % email_provider)
        sys.exit(1)
    
  • Instead of making the user type 'y' to run again every time, you could do something like this:

    while True:
        sendto = raw_input("Target address (or just hit ENTER to quit) > ")
        if sendto == '':
            sys.exit(0)
        to = CC = sendto
        ...
    
\$\endgroup\$
3
\$\begingroup\$

Naming

You may be using the smtplib library to help you talk to the SMTP server, but the object returned by smtplib.SMTP() is an SMTP client, not a server. When you call methods on that object, you are asking the client to say EHLO, initiate the TLS handshake, etc. Therefore, from a programming point of view, that object should be called smtpclient, not smtpserver.

Superfluous code

Prompting for the user's name and printing the machine's own hostname is completely irrelevant to the goal of sending e-mail, and in fact you never again refer to user and host_name after the first print statement.

smtpserver.ehlo() calls a method that makes the client initiate the SMTP handshake. However, smtpserver.ehlo a couple of lines further down accomplishes nothing.

The CC variable is never used, and is therefore misleading.

Overall structure

Prompting the user for the service provider and connecting to the SMTP server before main() is ever called is weird. By convention, if you have a function named main(), that's the entry point of your program, and the only code that runs outside the main() function is the code that calls it.

Starting the SMTP session on program startup is the wrong thing to do. If the user takes a long time to compose the mail message, then the server will disconnect the client after an inactivity timeout period has elapsed. Then, when the user finally wants to send the message, an smtplib.SMTPServerDisconnected exception will occur. You don't have any exception handling in your code.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.