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

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. — Aseem Bansal, Jul 22 '13 at 5:00
Why are you running ./killtv a bunch of times and then running another python script (test.py) from your python script? — raylu, Jul 26 '13 at 2:06
http get request opens a pyside window (test.py), killtv just closes the previous request/window — sirvon, Jul 26 '13 at 2:25
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. — Aseem Bansal, Jul 30 '13 at 13:12
@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. — sirvon, Aug 3 '13 at 1:32

Jamal · Accepted Answer · 2014-10-09 22:53:56Z

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.

tokland · Answer 2 · 2013-07-22 18:00:40Z

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)

asked	1 year ago
viewed	301 times
active	yesterday

current community

your communities

more stack exchange communities

Optimization of “def do_GET(self)”

2 Answers 2

Your Answer

Not the answer you're looking for? Browse other questions tagged python optimization or ask your own question.

Hot Network Questions

current community

your communities

more stack exchange communities

Optimization of “def do_GET(self)”

2 Answers 2

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged python optimization or ask your own question.

Related

Hot Network Questions