#!/usr/bin/python import subprocess import smtplib def CheckFile(): file = open('old_ip.txt', 'r') if ip != file.readline().strip(): file.close() open('old_ip.txt', 'w').close() file = open('old_ip.txt', 'w') file.write(ip) file.close() return True return False def SendMail(): content = ip mail = smtplib.SMTP('smtp.yandex.ru', 587) mail.ehlo() mail.starttls() mail.login('Your Yandex mail login', 'Your Yandex mail password') mail.sendmail('Your Yandex mail login', 'Your Yandex mail login', content) def GetIp(): open('new_ip.txt', 'w').close() subprocess.Popen('./telnet.sh').wait() GetIp() ip = open('new_ip.txt', 'r').readline().strip() if CheckFile() == True: SendMail()
It is usually not a good practice to hardcode specific python interpreter at specific path:
#!/usr/bin/python. Instead, use
#!/usr/bin/env python, which will interpret
$PATH and make your script more portable.
By looking at function name, stranger should roughly understand what the function does. If he doesn't, then the name should be changed.
"Early quit" is a common pattern which reduces branching and indents in python code. Instead of writing
if a: <1000 lines of code> else: <return / raise exception>, revert the condition:
if not a: <return / raise exception>. This way the "else" clause is not needed, and program logic is straightforward: if something is wrong, quit or raise, otherwise go on.
Side effects are evil: it's very hard to track what function does under the hood, because function signature does not reflect side effects. Always try to write "pure" functions, i.e. those which receive inputs, do something internally, and return output. You should be able to run such function million times and get exactly the same result with nothing changed in the environment. However, side effects are totally OK if that's the only thing the function does.
If some value is hidden somewhere in the middle of code, it will be very hard to find it later. Better solutions: 1) use environment variables 2) use constants in module's top section 3) use class variables.
This code is not really needed or may be simplified
If you want to just create a file, use
When you have some code in module scope, the code will be executed when someone imports that module - which makes the module completely non-reusable. Maybe it's not meant to be reusable today, but you'll have to refactor if you need to import the module later. Use
if __name__ == '__main__' to detect whether module was imported or used as a standalone script.
if a == True should be replaced with
if a is True, or better just
if a == False should be replaced with
if a is False, or better just
if not a. But be careful with cases when
a may be
None or is an object which has special rules of conversion to boolean.
if check_file(): # ...
Create new review request