Script for creating custom hosts files

Question

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))

Answer 1

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?