5
\$\begingroup\$

I wrote a basic Python script that consolidates and customizes several popular adblocking hosts files. I was interested in making it more efficient if possible, and I'd like to improve my style and best practices.

import urllib.request
import re
import os

os.chdir('C:/Windows/System32/drivers/etc')

# This contains a list of URLs where popular hosts files are located.
sources = [...]

def listDomains(sources):   
    domains = set()
    for s in sources:
        lines = list(urllib.request.urlopen(s))
        for l in lines:
            str = l.decode('utf-8')
            if isValid(str):
                d = getDomain(str)
                if isDomestic(d):
                    domains.add(d)
    return domains

def isValid(line):
    match = re.search('^(0\.0\.0\.0|127\.0\.0\.1)', line)
    if match:
        return True
    return False

def getDomain(str):
    return str.split()[1]

def isDomestic(domain):
    match = re.search('\.(com|net)$', domain)
    if match:
        return True
    return False

def makeHostsFile(domains):
    f = open('hosts', 'w')
    for d in domains:
        f.write('0.0.0.0 ' + d + '\n')
    f.close()

makeHostsFile(listDomains(sources))
\$\endgroup\$
4
\$\begingroup\$

Overall this looks pretty good, but there are some places where you can write more idiomatic Python. Some general comments:

  • More docstrings and comments would be nice. For example: what is a “domestic” URL, and why does it match com/net? Explain why the code is written this way – it will make things much easier if you have to debug/modify a program later.

  • Using single letter variable names is a bad habit. It’s better to use words, because they’re more descriptive and make the program easier to read. (They’re also easier to grep for later!)

  • The Python convention for variable names is lowercase_with_underscores, not dromedaryCase. (The exception is class names, which are PascalCase.)

Now some specific comments about your Python style:

  • Don’t use str as a variable name; this is the name of a builtin function. Overriding the names of the builtin functions is a bad practice, because it will cause all sorts of bugs later.

  • To avoid excessive indentation, I’d combine the two if statements at the end of listDomains into a single line:

    if is_valid(my_str) and is_domestic(get_domain(my_str)):
         domains.add(get_domain(my_str))
    
  • Rather than doing if match: return True; else: return False in isValid and isDomestic, you can just do return match. That’s shorter, easier to read and more idiomatic.

  • In getDomain(), you should explain the format of the line that you’re expecting, so that I know why you’re taking the element in index 1.

  • In makeHostsFile, rather than having explicit open() and close() calls, the Python idiom is to use a with statement:

    with open('hosts', 'w') as f:
        for domain in domains:
            f.write('0.0.0.0 {domain}\n'.format(domain))
    

    Note also that I’ve used new-style string formatting rather than string concatenation; this is the preferred way to do this sort of construction.

  • I would extend the makeHostsFile function to take an optional path to the hosts file, so that this code is more portable.

  • Rather than doing your program at top-level, wrap it in a main() function and call as follows:

    def main():
        import os
        hosts_file = 'C:/Windows/System32/drivers/etc/hosts'
        make_hosts_file(list_domains(sources), hosts_file)
    
    if __name__ == '__main__':
        main()
    

    The code inside main() is only executed if the script is run directly, but if you import this file from another program, it will be ignored. That makes it easier to reuse this code, because there aren’t any funny side-effects from importing it.

In terms of speed, the regexes seem pretty simple. Is there any reason you couldn’t just do

def is_valid(line):
    return ('0.0.0.0' not in line) and ('127.0.0.1' not in line)

def is_domestic(line):
    return line.endswith('.com') or line.endswith('.net')

which feels like it would probably be a lot faster?

\$\endgroup\$
  • \$\begingroup\$ Thanks for all your help and input. I really appreciate it. I don't think I can just check for 0.0.0.0 or 127.0.0.1 within a line, because sometimes the hosts-file author comments out a block-rule due to issues. So that would return false positives. But I can use startswith(). One other question: why would "return ('0.0.0.0' not in line) and ('127.0.0.1' not in line)" be preferable to "return ('0.0.0.0' in line) or ('127.0.0.1' in line)"? \$\endgroup\$ – amt528 Jul 6 '15 at 18:28
  • \$\begingroup\$ @amt528 Ah yes, I’d missed the caret. As for your second question… um, that might be a mistake on my part. Oops, you can drop the “not”s. \$\endgroup\$ – alexwlchan Jul 6 '15 at 18:41
  • \$\begingroup\$ Also, could you explain why import os should go in main? And based on your suggestions, should my function to make the host file have directory as a second parameter? \$\endgroup\$ – amt528 Jul 6 '15 at 23:05
  • \$\begingroup\$ @amt528 The “import os” thing is a mistake; I was using the os module in the hosts_file line, but I removed it and forgot to take out the import. In general, I put imports that are exclusive to the main() function within the function, just to keep them distinct from the rest of the code – I think of main() as a distinct block. \$\endgroup\$ – alexwlchan Jul 7 '15 at 20:27
  • \$\begingroup\$ NB. that "return match" isn't doing the same thing; match is a regex match object, not a boolean. return True if match else False is a neater way to convert it to a boolean in one line of Python where that matters. It doesn't exactly matter in your code, but it might in general. (But the return ('0.0.0.0' in line ... version is even neater. \$\endgroup\$ – TessellatingHeckler Jul 7 '15 at 23:51

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.