patternpythonMinor
Get external IP via email
Viewed 0 times
externalgetviaemail
Problem
I wrote a script for my Raspberry Pi that keeps checking my external IP. If it changes, the Raspberry Pi will send me the new IP via email.
Any suggestions for improvements?
Any suggestions for improvements?
#!/usr/bin/python3
import os, time
def getIP():
os.system('dig +short myip.opendns.com @resolver1.opendns.com > raspip.txt')
with open('raspip.txt', 'r') as f:
return f.readline().rstrip('\n')
def sendIP(currentIP):
os.system('sendEmail ' +
'-o tls=yes ' +
'-f \'***@outlook.com\' ' +
'-t \'***@outlook.com\' ' +
'-s \'smtp.live.com\' ' +
'-xu \'***@outlook.com\' ' +
'-xp \'**password**\' ' +
'-u \"raspberry via python\" -m ' +
'\"' + currentIP + '\"')
def main():
lastIP = None
while (True):
currentIP = getIP()
if (lastIP != currentIP):
sendIP(currentIP)
lastIP = currentIP
time.sleep(60)
if __name__ == "__main__":
main()Solution
You should probably prefer
You should use
Don't call
This is safe against quoting errors (what if the IP contained
You also didn't need the
You do:
It's more idiomatic to treat files as iterables, so instead one would do
This also lets you add a default if wanted with
You also don't need the
Don't put brackets around conditions in loops of
A more fancy example might use, say, Python's
#!/usr/bin/env python3 as it's less likely to vary on different systems. It's not a big deal, though.You should use
snake_case, even for IP, so names like get_ip and send_ip.Don't call
os.system; use subprocess.Popen instead:subprocess.Popen('dig +short myip.opendns.com @resolver1.opendns.com > raspip.txt'.split())subprocess.Popen([
'sendEmail',
'-o', 'tls=yes',
'-f', '***@outlook.com',
'-t', '***@outlook.com',
'-s', 'smtp.live.com',
'-xu', '***@outlook.com',
'-xp', '**password**',
'-u', 'raspberry via python', '-m',
current_ip
])This is safe against quoting errors (what if the IP contained
"?).You also didn't need the
+s; strings automatically concatenate. You don't need to escape 's inside "" or " inside '' - but it doesn't matter with Popen since you don't need to worry about quoting.You do:
with open('raspip.txt', 'r') as f:
return f.readline().rstrip('\n')It's more idiomatic to treat files as iterables, so instead one would do
with open('raspip.txt', 'r') as f:
return next(f).rstrip('\n')This also lets you add a default if wanted with
next(f, default). It might be a better idea to catch the error explicitly, since StopIteration can interfere with generators:with open('raspip.txt', 'r') as f:
try:
return next(f).rstrip('\n')
except StopIteration:
raise ValueError("No text in raspip.txt!")You also don't need the
'r' argument - it's default.Don't put brackets around conditions in loops of
ifs. It's just while True: and if last_ip != current_ip.A more fancy example might use, say, Python's
email library - but this is fine for what it does.Code Snippets
subprocess.Popen('dig +short myip.opendns.com @resolver1.opendns.com > raspip.txt'.split())subprocess.Popen([
'sendEmail',
'-o', 'tls=yes',
'-f', '***@outlook.com',
'-t', '***@outlook.com',
'-s', 'smtp.live.com',
'-xu', '***@outlook.com',
'-xp', '**password**',
'-u', 'raspberry via python', '-m',
current_ip
])with open('raspip.txt', 'r') as f:
return f.readline().rstrip('\n')with open('raspip.txt', 'r') as f:
return next(f).rstrip('\n')with open('raspip.txt', 'r') as f:
try:
return next(f).rstrip('\n')
except StopIteration:
raise ValueError("No text in raspip.txt!")Context
StackExchange Code Review Q#85298, answer score: 5
Revisions (0)
No revisions yet.