Status Report sending using python

Question

I used this script to sending status report to my team lead. It will read the status from the file Readme.md. Here i already wrote my status. if this script execute on target_time or greater than it will execute the mailer function otherwise ask for confirmation

# importing messagebox for confirmation diaglog box
import tkMessageBox
import smtplib

from time import gmtime, strftime


def mail_send():
    target_time = 02
    # By using strftime we only current time in hours, it returns string like
    # "12"
    current_Time = strftime("%H")
    print current_Time

    # convert str to number
    cTime = int(current_Time)

    def sub_mailer():

        print "Mailer loading ...."
        s = smtplib.SMTP('smtp.gmail.com', 587)
        s.starttls()
        s.login('[email protected]', 'anypass')
        f = open('Readme.md', 'r')
        filedata = f.read()
        subject = "Today Update"
        msg = 'Subject:{}\n\n{}'.format(subject, filedata)
        s.sendmail('[email protected]', '[email protected]',
                   msg)
        s.quit()
        f.close()

    # get time using time module
    if target_time <= cTime:
        sub_mailer()
    else:
        status = tkMessageBox.askquestion(
            "Shift hours is not completed. Shall i  send a mail", icon="warning")
        if status == "yes":
            sub_mailer()
        else:
            print "Mail sending process was canceled by user"

Answer 1

First of all, Welcome to CodeReview!

Let's analyse the code to see what could be improved here.

current_Time = strftime("%H")
print current_Time
# convert str to number
cTime = int(current_Time)

A few things could be improved here:

• Too many calls! current_Time = int(strftime("%H")) would have sufficed
• the comment # convert str to number adds no real value to the code readability

s = smtplib.SMTP('smtp.gmail.com', 587)

• Use better names, s does not make it clear what it does.
• Avoid "magic" numbers-- it would have been better to have these as named constants

f = open('Readme.md', 'r')

• Use context managers (with open ... as ...), see the docs

---

Some random thoughts

• Use guards-- that way your script can be imported by other modules. Might not need it here, but I think it is good practice to always use them.

• Consider upgrading your python-2.7 to python-3 and port all your code, since the Python 2 End Of Life is just 2 years away!

Answer 2

Ludisposed probably deserves the accept, just wanted to point out a few other pet peeves of mine.

• Never store credentials in your code.

  This line:

  s.login('[email protected]', 'anypass')
  

  is a disaster waiting to happen. All it takes for the whole Internet to get your credentials is for for you to forget to sanitize it one time. Your code should read the credentials from a config file or something similar. See configparser (2.7, 3.x) module for one way to do it, or _winreg (2.7) / winreg (3.x) module if you're an all-Windows shop.

• In fact a lot of your constants should be read from a config.

  target_time, the mail server and port, the path to the file you're scanning, subject, from and to emails. Much easier to edit a config than to have to scan your code for the proper places to edit.

• Functions should perform one job

  Your mail_send() function takes no parameters and does all the work. It would be better for it to take parameters corresponding to all of the config items, and then have another function e.g. main() get the configuration details and then pass them as parameters to mail_send(). The general principle is called the single responsibility principle. This keeps functions shorter thus easier to understand. They also end up easier to test, debug, edit, reuse, etc. Ideally each function should be able to fit on your screen so you can take it in at a glance.

• Inconsistent user interface

  Some of your code uses print, others use a tk message box. What's the reason for this? If you have to pass this onto someone else, will they expect and be able to intuitively comprehend this?

  I'm generally against print anyway, unless the function of the code is to generate output. For status messages (I believe all of your print) you ought to be using the logging (2.7, 3.x) module.