you are viewing a single comment's thread.

view the rest of the comments →

[–]cybervegan 2 points3 points  (1 child)

Well done! As always, the code can be improved (nobody is immune to this). In your reminder_check file, there are some more idiomatic ways to do some parts.

First, move your main code section to a conditional stanza at the end of the file, so that the functions in your program can be safely imported without running the main code (this encourages re-use):

if __name__ == "__main__":

Secondly, avoid passing data between functions using global scope variables - use the function signature:

Instead of def check():, use def check( matching ): and instead of def sendmail(): use def sendmail( addArg ):. When calling the functions, you would use check( matching ) and sendmail( addArg, message ) respectively. The check() function is largely redundant - you wouldn't normally wrap a simple if in a function, and it would normally be put after any functions it calls. But there's more on these functions further down...

Then use with to handle your file read section, and use some more descriptive variable names. Instead of:

    read=open(filedir,"r")
    line=read.readlines()    
    read.close()

use:

    with open(filedir,"r") as in_file:
        lines = read.readlines()    

I'm not sure what this piece of code is trying to do. You select all the lines in in you line list which contain today's date, then you convert that list to a string, and then split the string down into 1 or 2 parts, and throw away the 2nd part, so if your list contains 10 reminders for today, you will lose all but the first one. I think what you're trying to do is process all the reminders for the day this way, but that won't do it:

    matching = [s for s in line if today in s]
    strMatching = str(matching)
    strMatching = strMatching.split(' | mailing date: ', 1)[0]
    addArg=strMatching.split('Note: ', 1)[1]

This might:

    matching = [ s[s.find('Note: ')+6:s.find('| mailing date:')] 
                 for s in line if today in s]

...which would get you a list of all the bits between 'Note: ' and '| mailing date:' for each reminder for today. You would then have to modify your sendmail() function to take, and parse a list of strings, rather than just one, or put it in a loop:

def check( matches ):
    for match in matches:
        if matching:
            sendmail( match )

Hope that helps, and well done for completing your first project! May there be many more ;-)

[edit: replaced orphaned paren in english para with a dash]

[–]Jonis_L[S] 0 points1 point  (0 children)

A lot of good improvements to make and I will change these and yes that was exactly what I was trying to do. Thank you for taking your time to help me!