Optimization of "def do_GET(self)"

Question

This code works nicely for my purposes, but I would like to improve it exponentially.

What are some steps I can take? I'm interested in optimization, refactoring, and clean-code.

 def do_GET(self):
 qs = {}
 path = self.path
 print path
 if path.endswith('robots.txt'):
 self.send_response(500)
 self.send_header('Connection', 'close')
 return
 if '?' in path:
 path, tmp = path.split('?', 1)
 qs = urlparse.parse_qs(tmp)
 print path, qs
 if 'youtube' in path:
 ID = qs.get('vID')
 LTV = qs.get('channelID')
 EMAIL = qs.get('uEmail')
 TIME = qs.get('Time')
 userID = qs.get('uID')
 URL = qs.get('contentUrl')
 channelID = str(LTV)[2:-2]
 print channelID
 uEmail = str(EMAIL)[2:-2]
 print uEmail
 ytID = str(ID)[2:-2]
 print ytID
 ytURL = str("https://www.youtube.com/tv#/watch?v=%s" % ytID)
 print ytURL
 uTime = str(TIME)[2:-2]
 print uTime
 uID = str(userID)[2:-2]
 print uID
 contentUrl = str(URL)[2:-2]
 print contentUrl
 if sys.platform == "darwin":
 subprocess.Popen('./killltv', shell=True)
 sleep(1)
 subprocess.Popen('./killltv', shell=True)
 sleep(1)
 subprocess.Popen('./test.py %s'%ytURL, shell=True)
 elif sys.platform == "linux2":
 subprocess.Popen('./killltv', shell=True)
 sleep(1)
 subprocess.Popen('./test.py %s'% ytURL, shell=True)
 #subprocess.Popen('chromium-browser https://www.youtube.com/tv#/watch?v=%s'% ytID, shell=True)
 sleep(2)
 #subprocess.Popen("./fullscreen", shell=True)
 elif sys.platform == "win32":
 os.system('tskill chrome')
 browser = webbrowser.open('http://youtube.com/watch?v="%s"'% ytID)
 log.warn("'%s':,video request:'%s' @ %s"% (channelID, ytID, time))
 #playyt = subprocess.Popen("/Users/Shared/ltv.ap

• Aseem Bansal: Some explanation of its purpose would be helpful. That way we don't accidentally break it. That is the first feedback. Always use docstring at the beginning of a function explaining its purpose.
• raylu: Why are you running ./killtv a bunch of times and then running another python script (test.py) from your python script?
• sirvon: http get request opens a pyside window (test.py), killtv just closes the previous request/window
• Aseem Bansal: About your os specific code why are you only opening the browser when sys.platform is win32? If you add certain details about what you are trying to do by test.py then the os specific code can be converted into loops based on values stored in dictionaries with key being sys.platform.
• sirvon: @AseemBansal can you elaborate... I think this is something i'm searching for. test.py opens up a pyside window with a url passed in from the get request.

Answer 1

How about using

IS, LTV, EMAIL, TIME, userID, URL = map(\
 qs.get, ['vID', 'channelID', 'uEmail', 'Time', 'uID', 'contentUrl'])
channelID, uEmail, ytID, uTime, uID, contentUrl = map(\
 lambda x : str(x)[2:-2], [LTV, EMAIL, ID, TIME, userID, URL])
for i in (channelID, uEmail, ytID, uTime, uID, contentUrl):
 print i
ytURL = str("https://www.youtube.com/tv#/watch?v=%s" % ytID)
print ytURL

instead of

ID = qs.get('vID')
LTV = qs.get('channelID')
EMAIL = qs.get('uEmail')
TIME = qs.get('Time')
userID = qs.get('uID')
URL = qs.get('contentUrl')
channelID = str(LTV)[2:-2]
print channelID
uEmail = str(EMAIL)[2:-2]
print uEmail
ytID = str(ID)[2:-2]
print ytID
ytURL = str("https://www.youtube.com/tv#/watch?v=%s" % ytID)
print ytURL
uTime = str(TIME)[2:-2]
print uTime
uID = str(userID)[2:-2]
print uID
contentUrl = str(URL)[2:-2]
print contentUrl

The first one just prints ytURL at the end but it can be changed easily if you really want. Other than that they do the same thing. But I am sure the first one is much easier to modify and maintain.

Other than that I can only say after I know what this program is about.

It might also be a good idea to keep os-specific functions(like you are doing after checking for the os) into separate functions. It would make maintaining and modifying the code much easier.

You should also read PEP8, particularly the part about code-layout. Your indentation is not good.

What I meant was that this can be converted into

if sys.platform == "darwin":
 subprocess.Popen('./killltv', shell=True)
 sleep(1)
 subprocess.Popen('./killltv', shell=True)
 sleep(1)
 subprocess.Popen('./test.py %s'%ytURL, shell=True)
elif sys.platform == "linux2":
 subprocess.Popen('./killltv', shell=True)
 sleep(1)
 subprocess.Popen('./test.py %s'% ytURL, shell=True)

this equivalent structure

OS_DETAIL = { 'darwin' : 2, 'linux2' : 1}
for i in xrange(OS_DETAIL[sys.platform]):
 subprocess.Popen('./killltv', shell=True)
 sleep(1)
else:
 subprocess.Popen('./test.py %s'% ytURL, shell=True)

It doesn't solve the complete problem because I am not sure whether you'll need those commented statements or not. If the sleep(2) is actually needed in case of linux2 then you can store a tuple in the OS_DETAILS dictionary. But the problem with this approach is your win32 part. It is totally different. It can be taken care of using an structure like this.

if sys.platform in ['darwin', 'linux2`]:
 #place the code I have just suggested for these 2 
elif sys.platform == "win32"
 #place your code for win32

But as you can see this isn't the best way. It is better than the current solution but if the code for win32 can be refactored in some way (I don't know its functionality so don't ask) to fit the same pattern then it all comes down to a simple dictionary and a loop structure.

Answer 2

Some notes about your code:

• Use 4 spaces for indentation.
• Local variables should use the underscore style: name_of_variable.
• This function is too long. Split code in functions.
• When writing conditionals, try to keep a if/elif/else layout, without early returns, you'll see that readability is greatly enhanced.
• Use dictionaries to hold multiple named values, not one variable per value, that's too messy.
• Some patterns are repeated all over the place, abstract them.
• Don't update variables in-place unless there is a good reason for it (99% of the time there isn't).

I'd write (show_video has been left out, it's clearly another function's task):

def do_GET(self):
 url = urlparse.urlparse(self.path)
 if 'youtube' not in url.path:
 sys.stderr.write("Not a youtube path: {0}".format(self.path))
 elif url.path == '/robots.txt':
 self.send_response(500)
 self.send_header('Connection', 'close')
 else:
 url_params = urlparse.parse_qs(url.query)
 keys = ["vID", "channelID", "uEmail", "Time", "uID", "contentUrl"]
 params = dict((key, url_params[key][2:-2]) for key in keys)
 yt_url = "https://www.youtube.com/tv#/watch?v={0}".format(params["vID"])
 show_video(yt_url)