2
\$\begingroup\$

I made a little Python3 script to grab video clips from a website for viewing off-line later and would like your critique, advice, tips & tricks and/or learning resources to create better code. (Project on GitHub)

#!/usr/bin/env python3
''' phgrab.py: clip downloader. '''

import os
import sys
import argparse
import urllib.request
import bs4


def reporthook(blocknum, blocksize, totalsize):
    ''' Define the reporthook for url-retrieval
    (displays info while grabbing). '''
    readsofar = blocknum * blocksize
    if totalsize > 0:
        percent = readsofar * 1e2 / totalsize
        display = "\r%*d / %d %5.0f%%" % (
            len(str(totalsize)), readsofar, totalsize, percent)
        sys.stdout.write(display)
        if readsofar >= totalsize:
            sys.stdout.write("\n")
    else:   # total size unknown
        sys.stderr.write("read %d\n" % (readsofar,))


PARSER = argparse.ArgumentParser(
    description="a commandline clip downloader.")
PARSER.add_argument('clipcode', nargs='+',
                    help="a ten-digit clipcode.")
PARSER.add_argument('-p', '--proxy',
                    help="A http proxy to use; e.g. http://localhost:8000")

PROXY = PARSER.parse_args().proxy
CLIPCODES = PARSER.parse_args().clipcode

if PROXY:
    print("Grabbing through http proxy: %s\n" % PROXY)
    os.environ['http_proxy'] = PROXY

for i in CLIPCODES:
    if not i.isdigit() or not len(i) == 10:
        sys.exit(
            "'\033[1m%s\033[0m' is not a valid ten-digit clipcode," % i,
            "please check your input and try again.")

for i in CLIPCODES:
    soup = bs4.BeautifulSoup(
        urllib.request.urlopen("http://somesite.com/%s" % i))
    title = soup.find("h1", class_="video-title").string
    href = soup.find("a", class_="download button")['href']
    print("Grabbing \033[1m%s\033[0m:" % i)
    local_filename, headers = urllib.request.urlretrieve(
        href, '%s - %s.mp4' % (i, title), reporthook)
    print("\033[1m", local_filename, "Done\033[0m")
\$\endgroup\$
  • 4
    \$\begingroup\$ I felt that the actual used site was not very relevant to your code question, so I decided to edit your post and remove the actual site. This code will probably work for more than one site. \$\endgroup\$ – Simon Forsberg Jan 30 '15 at 15:25
  • \$\begingroup\$ Hi @simon-andré-forsberg, I wrote this script for this site and did not (yet) consider usability for other sites and, at first glance, have not found other use-cases. I'll check it out, Thanks! \$\endgroup\$ – markhamden Jan 30 '15 at 21:44
  • \$\begingroup\$ Either way, you should be careful with what links you spread. \$\endgroup\$ – Simon Forsberg Jan 30 '15 at 21:49
3
\$\begingroup\$

A bug

This just won't work:

sys.exit(
    "'\033[1m%s\033[0m' is not a valid ten-digit clipcode," % i,
    "please check your input and try again.")

sys.exit takes a single integer parameter to use as the status code of the program, so that callers can check if the program terminated normally or failed. I suppose you wanted to print instead and then sys.exit(1).

Move all code to methods

Don't keep code in the global namespace. A good practice is to move everything to methods, and call them from inside a if __name__ == "__main__": guard, like this:

def parse_args():
    # arg parser code

def main():
    args = parse_args()
    proxy = args.proxy
    clipcodes = args.clipcode

if __name__ == "__main__":
    main()

This way it will be possible to import the module into a larger app later. Another advantage is that it doesn't pollute the global namespace, which could lead to obscure bugs.

Use print() if you want to print

Printing with sys.stdout.write is very unusual and tedious. Using print() is more common and simple. If you don't want to print a newline, use the end='' flag, for example:

print(message, end='')

Naming

i is really a very poor name in this loop:

for i in CLIPCODES:
    if not i.isdigit() or not len(i) == 10:
        # ...

It would be better to use something more descriptive, for example clipcode.

Another thing here, not len(i) == 10 is a very odd way of writing len(i) != 10.


It's very unusual to see ALL_CAPS for variable names that are not global constants. For example DEFAULT_PROXY_PORT = 8080 would make a fine global constant, but these look really odd:

PARSER = argparse.ArgumentParser(...)
CLIPCODES = PARSER.parse_args().clipcode
\$\endgroup\$
  • \$\begingroup\$ Hi @janos, many thanks for your help & advice! I'll implement & update (and read some more about the aforementioned topics) ASAP. \$\endgroup\$ – markhamden Jan 30 '15 at 21:54

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.