6
\$\begingroup\$

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?

#!/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()
\$\endgroup\$
1
  • \$\begingroup\$ How set are you on using email? 1. DNS: If you manage your DNS and DHCP yourself, you can assign a hostname to your pi in the same way that a provider like no-ip does. This would obviously be local only, and if you absolutely need to use an IP, you could just do a quick dig. 2. Pushbullet: A quick bash script can send you the IP as a push to any of your other devices. Pushbullet API documentation \$\endgroup\$ – Wilhelm Erasmus Mar 29 '15 at 11:30
5
\$\begingroup\$

You should probably prefer #!/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.

\$\endgroup\$
2
\$\begingroup\$

Do away with file.

import subprocess as sp

def get_ip():
    return sp.getoutput('dig +short myip.opendns.com @resolver1.opendns.com').split()[0]
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.