Download URL from clipboard

Question

This is a Python script I made to download a URL from the clipboard.

When a new URL is copied to a clipboard it adds the URL to a download queue, then on a separate thread it downloads the URL to a file.

I am new to multithreading. Is my script properly using multithreading? It needs to download the data while still listening for changes in the content of the clipboard.

import time
import os
import urllib
import subprocess
import threading

download_number = 0
count=0
save_file = "links_list.txt"
old_url = ""
download_queue = []
download_delay = 5

data_file = open(save_file, "r")
past_links = data_file.read()
data_file.close()

def get_image():
    print download_queue
    url = download_queue.pop(0)

    data = urllib.urlopen(url).read()
    name = url.split("/")[-1:][0]
    extension = name.split(".")[-1:][0]
    basename = name.split(".")[:-1][0]

    # make a new file name
    if os.path.exists(name):
        new_name = basename+"-0."+extension
        try_count = 0
        while os.path.exists(new_name):
            try_count += 1
            new_name = basename+"-"+str(try_count)+"."+extension
        name = new_name

    data_file = open(name, "w")
    data_file.write(data)
    data_file.close()


    # goto the next link
    if len(download_queue) != 0:
        time.sleep(download_delay)
        get_image()


while(1):
    time.sleep(0.25)
    clipboard = subprocess.check_output('pbpaste', shell=True)
    # check if the clipbard contains a new url
    if clipboard[:4] == "http" and clipboard != old_url and clipboard not in past_links:
        past_links += clipboard
        old_url = clipboard

        # add url to queue
        download_queue.append(clipboard)
        print download_number,

        # start the download fuction
        t = threading.Thread(target=get_image)
        t.start()

        # links that have been downloaded
        data_file = open(save_file, "a")
        data_file.write(clipboard+"\n")
        data_file.close()

        download_number += 1

Answer 1

• You maintain old_url as well as past_links. You probably don't need old_url at all because everything you process will be added to past_links.

• You are reading past_links as one giant string with embedded newlines. That won't be very efficient when doing clipboard not in past_links because it will have to search through the whole string.

• When you run past_links += clipboard, you are sticking the data on to the end of past_links with no newline at the end, so new links added there will be jammed together.

• You should probably set up past_links as a set, and use set operations. You can still read and write it as a plain text file.

• You create a new thread for each download request, which will download multiple files in parallel. It's not clear that is what you really want to do.

• Your use of a recursive call to get_image() is a very strange way to implement a loop.

• You are reading the entire download into memory in one giant string, and then writing it out to disk. If you were to download a very large file, your script would require a corresponding amount of memory during downloading. Use urllib.urlretrieve to read directly into a file.

• Use clipboard.startswith("http") instead of clipboard[:4] == "http"

• Finally, use the standard Python Queue module instead of implementing your own:

  The Queue module implements multi-producer, multi-consumer queues. It is especially useful in threaded programming when information must be exchanged safely between multiple threads.

Answer 2

I won't touch on the things @GregHewgill did in his answer. I'll talk about some of general Python things I noticed.

• You have a section of code where you get the name, extension and basename of the URL. You use this syntax:

  name = url.split("/")[-1:][0]
  

  This is overly complicated. The split is fine. The slicing is what is not needed. array[-1:] says 'Give me a chuck of array starting at the last index and going until the end of the array'. You could easily get the info you want by simply:

  name = url.split('/')[-1]
  

  To get the next two pieces of information is quite easy. os.path.splitext easily does the splitting work for you:

  basename, ext = os.path.splitext(name)
  

  NOTE: This leaves the . on the extension.

  ---

• The next section of code where you create new_name:

  if os.path.exists(name):
      new_name = basename+"-0."+extension
      try_count = 0
      while os.path.exists(new_name):
          try_count += 1
          new_name = basename+"-"+str(try_count)+"."+extension
  

  can be simplified as well. Use str.format to create new_name and move everything into the while:

  if os.path.exists(name):
      try_count = 0
      new_name = ''
      while os.path.exists(new_name):
          new_name = '{}-{}{}'.format(basename, try_count, extension)
          try_count += 1
  

---

• When dealing with files, use the with syntax. It will automatically close the file once the program leaves that block:

  with open('my_file.txt', 'r') as file:
      # Do stuff.
  
  # file is closed at this point.
  foo = 'Hello World!'
  

---

• if len(download_queue) != 0: can be written as if download_queue. This is because an empty list evaluates to `False:

  >>>if []:
  ...    print('True')
  ...else:
  ...    print('False')
  ...
  False