Lex Hider enlightened us with: > Any tips on the code quality and use of python would be appreciated. > I've got a feeling the overall structure is up the creek.
I'll post some remarks about the code ;-) > HOME = os.path.expanduser("~") I wouldn't use this. Just use os.environ['HOME']. In most cases it turns out to be the same directory, but it adds more flexibility. If someone wants your app to read/write to another directory, he/she can simply change the HOME environment variable. > # user configurable > #maxChecksPerDay = 8 > #maxChecksPerDay = 12 > maxChecksPerDay = 24 > myTemp = '/tmp' > #podDir = os.path.join(HOME, 'Audio/Podcasts') > podDir = os.path.join(HOME, 'Podcasts') > # end user configurable A bit of nitpicking: place a blank line between the user configurable part and the rest of the code. > def exitFunc(): > #f.close() > #log.close() > if boz: > print boz Write function comments! Use the docstring to describe your function - what does it do, and what does it return? Do this for all functions you write. > # render is used because feeds use a lot of html, not just plain text. > def render(html): > if html: > html = re.sub('"', '\\"', html.encode('utf8')) Use a raw string for the second argument to make it more readable: html = re.sub('"', r'\"', html.encode('utf8')) > command = 'echo "' + html + '" | lynx -dump -stdin -force_html' > os.system(command) Use the subprocess module or the popen2 module to open lynx. That way, you can feed the HTML to lynx directly, and you're not bound to the maximum line length of the shell. It also removes the need to escape quotes. > def updateCache(feeds): > l = [] Use longer names than 'l'. > print "updating local xml cache..." > for feed in file(feeds, "r").read().split('\n'): > if not re.match('^http://', feed): # feedList ignores > # anything but urls Here you say you only match URLs starting with "http://", but at the start you claimed to only use URLs starting with "http". Be sure to keep your documentation and your code in sync. > def htmlTitle(mainTitle, subTitle): > s = '<HR>' > s += '<H2>' + mainTitle + '</H2>' > s += '<H3>' + subTitle + '</H3>' > return s It might be easier on the eyes if you use: s = '<hr><h2>%s</h2><h3>%s</h3>' % (mainTitle, subTitle) It might also be wise to use lower-case HTML tags to remain compatible with XHTML. > def downloadPod(url, dest): > kb = 2 > success = 0 > command = 'wget --continue -O "' + dest + '" "' + url + '"' > status = os.system(command) Here you pass the arguments of the function to a system call. This means that before the downloadPod function is called, the 'url' and 'dest' should have been escaped or cleared of unwanted characters. This should really be documented. Overall, your code needs to be commented and documented much better. Think of other people reading the code, and explain _why_ you do something, instead of explaining _what_ you're doing. Sybren -- Sybren Stüvel Stüvel IT - http://www.stuvel.eu/ -- http://mail.python.org/mailman/listinfo/python-list