6
\$\begingroup\$

I feel a need to rewrite a rather large Python class that "does its job" but it looks somewhat terrible since it was pasted together while learning Python and the platform Google App Engine. It works so that there are no bugs towards user but readability and structure are poor. This is also the largest class of the whole project that posts entities to the datastore doing the basic function "ADD" for our entities and related blobs:

class AdLister(FBBaseHandler, I18NHandler): 

    def post(self, view): 
        logging.info('in adlister post')
        message = ''
        challenge = self.request.get('recaptcha_challenge_field').encode('utf-8')
        response  = self.request.get('recaptcha_response_field').encode('utf-8')
        remoteip  = os.environ['REMOTE_ADDR']
        cResponse = captcha.submit(
                     challenge,
                     response,
                     CAPTCHA_PRV_KEY,
                     remoteip)
        if cResponse.is_valid==True:
            isHuman=True
        else:#failed anti-spam test and can try again
            isHuman=False   
            data = AdForm(data=self.request.POST)
            #Reprint the form
            import util
            template_values = {'logo':'montao' if util.get_host().endswith('.br') else 'koolbusiness','isHuman':isHuman,'form':data,'user_url': users.create_logout_url(self.request.uri) if users.get_current_user() else 'login','user' : users.get_current_user(),}
            template_values.update(dict(current_user=self.current_user, facebook_app_id=FACEBOOK_APP_ID))
            template_values.update(dict(capture=captcha.displayhtml(public_key = CAPTCHA_PUB_KEY, use_ssl = False, error = None)))
            path = os.path.join(os.path.dirname(__file__), 'market', 'market_insert.html')
            self.response.out.write(template.render(path, template_values))           
            return            

        from datetime import datetime
        from i18n import FBUser
        from twitter_oauth_handler import OAuthClient
        import random
        twenty = random.randint(1,5) > 3
        def getGeoIPCode(ipaddr):
            from google.appengine.api import memcache
            memcache_key = "gip_%s" % ipaddr
            data = memcache.get(memcache_key)
            if data is not None:
                return data
            geoipcode = ''
            from google.appengine.api import urlfetch
            try:
                fetch_response = urlfetch.fetch('http://geoip.wtanaka.com/cc/%s' % ipaddr)
                if fetch_response.status_code == 200:
                    geoipcode = fetch_response.content
            except urlfetch.Error, e:
                pass
            if geoipcode:
                memcache.set(memcache_key, geoipcode)
            return geoipcode
        path = os.path.join(os.path.dirname(__file__), 'market', 'credit.html') #consume
        lat=0
        lng=0
        try:
            lat = self.request.POST.get('lat')#get lat,lt or latitude
            lng = self.request.POST.get('lng')#get lng, ln or longitude

        except Exception:
            if False:#self.request.POST.get('montao_id'):
                lat=-23.7126
                lng=-46.6415
            else:
                pass
        if False:#self.request.POST.get('montao_id'):
            lat=-23.7126
            lng=-46.6415      
        try:
            ad = Ad(location=db.GeoPt(lat, lng))
            ad.update_location()#also runs this at edits
        except Exception:
            ad = Ad()

        import util          
        if util.get_host().find('koolbusiness') > 0:
            url = 'www.koolbusiness.com'            
        if util.get_host().find('lassifiedsmarket') > 0:
            url = 'classifiedsmarket.appspot.com'        
        if util.get_host().find('montao') > 0 or util.get_host().find('localhost') > 0:
            path = os.path.join(os.path.dirname(__file__), 'market', 'market_ad_preview.html')
            ad.url ='montao'
            logo = 'montao'   

        if (util.get_host().find('acktuellt') > 0 or twenty):
             asked_question = Question(question=self.request.get('title'), asker=db.IM("xmpp", 'classifiedsmarket.appspot.com'))#add urn
             asked_question.put()
             #path = os.path.join(os.path.dirnam e(__file__), 'market', 'market_ad_preview.html')
             if (not twenty or util.get_host().find('acktuellt') > 0):
                ad.published = False             
             path = os.path.join(os.path.dirname(__file__), 'market', 'credit.html') 
        ad.ip = self.request.remote_addr
        ad.ipcountry = getGeoIPCode(self.request.remote_addr)
        if users.get_current_user():
            ad.user = users.get_current_user()
        try:
            ad.type = self.request.POST.get('type')
        except Exception:
            pass
        ad.title = self.request.POST.get('title')
        try:
            ad.text = self.request.POST.get('text')
        except Exception:
            pass
        try:
            ad.currency = self.request.POST.get('currency')
        except Exception:
            pass 
        try:
            ad.facebookID = int(self.current_user.id)
        except Exception, ex:
            logging.debug("failed creating facebookID object for user %s", str(ex))
            pass        

        try:
           client = OAuthClient('twitter', self)
           info = client.get('/account/verify_credentials')
           ad.twitterID = int(info['id'])
        except Exception, ex:
            logging.debug("creating twitter object for user failed %s", str(ex))
            pass      
        try:
            lat = self.request.POST.get('lat')#get lat,lt or latitude
            lng = self.request.POST.get('lng')#get lng, ln or longitude
            ad.geopt = db.GeoPt(self.request.POST.get('lat'),self.request.POST.get('lng'))#above
            ad.geohash = Geohash.encode(float(lat),float(lng), precision=2)#evalu8 precision variable
        except Exception:
            pass
        try:
            ad.category = self.request.POST.get('cg')             
        except Exception:
            logging.error(Exception)
        if self.request.POST.get('company_ad')=='1':
            ad.company_ad = True
        try:
            ad.url=url
        except Exception:
            pass 
        try:
            ad.phoneview = self.request.get('phone_hidden',None) is not None    
        except Exception:
            pass             
        try:
            ad.place = self.request.POST.get('place')
            ad.postaladress = self.request.POST.get('place')
        except Exception:
            pass          
        try:
            ad.name = self.request.POST.get('name')
        except Exception:
            pass 
        try:
            ad.email = self.request.POST.get('email')
        except Exception:
            pass 
        try:
            ad.phonenumber = self.request.POST.get('phonenumber')
        except Exception:
            pass 
        ad.price = self.request.POST.get('price')
        ad.save()        
        self.s = session.Session()
        self.s['key'] = ad.key()         
        def create_image(number, self, file, ad):
            logging.debug('creating image')            
            try:

              file_name = files.blobstore.create()
              with files.open(file_name, 'a') as f:
                  f.write(file)
              files.finalize(file_name)
              blob_key = files.blobstore.get_blob_key(file_name)
              logging.debug('creating image')
              img = Image(reference=ad) 
              logging.debug('creating image')
              img.primary_image = blob_key
              logging.debug('creating image')
              img.put()
              ad.put()
            except Exception:                
               self.response.write(Exception)
               #pass            
            #deprecate see http://stackoverflow.com/questions/5863305/storing-mime-type-with-blobstore
            try:
                filedata = file                
                im = Image(reference=ad)       
                im.full = filedata
                im.small = images.resize(filedata, 640, 480)                 
                tmp=images.Image(im.full)       
                if tmp.width > 80:                
                    im.thumb = images.resize(filedata, 80, 100)#don't if already small                   
                file_name = self.request.POST.get(number).filename                

                im.name = file_name
                im.published    = True
                n = im.name.lower()
                ext = 'jpg'
                for e in ['jpeg','jpg','png','tif','tiff','gif','bmp']:
                    if n.endswith(e):
                        ext=e
                im.full_ext=ext                
                im.small_ext=ext
                im.thumb_ext=ext  
                im.save()       
            except Exception:                
               #self.response.write(Exception)
               pass
        #end deprecate
        try:
            create_image('file', self, self.request.POST.get('file').file.read(), ad)
        except Exception:
            pass      
        try:
            create_image('file2', self, self.request.POST.get('file2').file.read(), ad)
        except Exception:
            pass 
        try:
            create_image('file3', self, self.request.POST.get('file3').file.read(), ad)
        except Exception:
                pass 
        try:   
            create_image('file4', self, self.request.POST.get('file4').file.read(), ad)
        except Exception:
            pass 
        try:
            create_image('file5', self, self.request.POST.get('file5').file.read(),ad)
        except Exception:
                pass         
        try:
            ad.set_password(self.request.POST.get('password'))                 
        except Exception:
            size = 9
            vowels='aeiou'
            consonants='bcdfghjklmnpqrstvwxyz'
            password=''
            from random import randint
            from random import choice
            import random
            minpairs = 4
            maxpairs = 6
            for x in range(1,random.randint(int(minpairs),int(maxpairs))):
                consonant = consonants[random.randint(1,len(consonants)-1)]                      
                if random.choice([1,0]):
                    consonant=string.upper(consonant)
                password=password + consonant
                vowel = vowels[random.randint(1,len(vowels)-1)]
                if random.choice([1,0]):
                    vowel=string.upper(vowel)
                password=password + vowel
                newpasswd = password                                 
            ad.set_password(newpasswd)         
        ad.put()
        key = ad.key()        
        matched_images=ad.matched_images             
        try:    
            im_from = db.IM("xmpp", 'classifiedsmarket.appspot.com')
        except Exception:
            pass    
        #"Added %s."
        msg =  _("Added %s.") % str(ad.title.encode('utf-8'))        
        self.response.out.write(template.render(path, {'url':util.get_host(),'message': msg ,'user':users.get_current_user(),  'ad.user':ad.user,  'ad':ad, 'matched_images':  matched_images,}) )
\$\endgroup\$
5
\$\begingroup\$

My first impulse would be to take care of repeating

try:
   ...
except Exceptiopn:
   pass

structures that take a lot of space e.g.

def value_of(self, param):
   try:
      return self.request.POST.get(param)
   except Exception:
      return None

...

ad.email = self.value_of('email')

or even better (as provided by James Khoury):

ad.email = self.request.POST.get(param, None)

and

def create_images(self, ad, *files):
   for file in files:
       try:
          create_image(file, self, self.request.POST.get(file).file.read(),ad)
       except Exception: 
          pass

...

self.create_images(ad, 'file1','file2','file3','file4','file5')

The second thing I would do is try to divide the long function to multiple functions that can be sensibly named. Possible functions include

  • generate_password
  • is_submitter_human
  • create_facebook_id
  • create_twitter_id
  • geolocate

When the basic functions have defined I would go on and try to write the post function in terms of higher abstractions that use the functions and let me read the intent of it as it was an executable comment.

def post(self, view): 
   if not self.is_submitter_human():
      return
   self.geolocate()
   ad = self.create_ad()
   self.populate_ad_with_request_parameters(ad)
   self.connect_with_facebook(ad)
   self.connect_with_twitter(ad)
   self.create_images(ad)
   self.set_password(ad)
   self.save(ad)
   self.render(ad);

Disclaimer: I don't know Python

\$\endgroup\$
  • \$\begingroup\$ I agree. That was also my spontaneous reaction that Iäm wasting a lot of statement just making a try / except where it might not be needed. Thank you also for the other concrete advice and that you actually read the code. \$\endgroup\$ – Niklas Aug 17 '11 at 19:52
  • 2
    \$\begingroup\$ Why not instead of def value_of(self, param): try: return self.request.POST.get(param) except Exception: return None use self.POST.get(param, None) it does the same thing? \$\endgroup\$ – James Khoury Aug 18 '11 at 23:25
  • 1
    \$\begingroup\$ In my defense: I did write a disclaimer. \$\endgroup\$ – Aleksi Yrttiaho Aug 19 '11 at 9:06
  • \$\begingroup\$ Excellent review thanks to Aleksi and James. I needed this. :-) \$\endgroup\$ – Niklas Aug 20 '11 at 13:52
5
\$\begingroup\$

Have a look at PEP-8. Among the guidelines that it offers is that you get all your imports in the beginning of the file. There is even a tool named pep8.py that you should run on your code that alerts you of some of the unfollowing of those guidelines.

One other note, on this line:

if cResponse.is_valid==True:

True isn't needed at all. Just leave it as:

if cResponse.is_valid:
\$\endgroup\$
4
\$\begingroup\$

Start with writing unit and/or feature tests that capture the current behaviour. Then begin refactoring while making sure that the tests pass all the time.

If you are not an experienced developer "refactoring" might be a bit vague; I'd then suggest to simply start with removing duplication in whatever form there is. Aleksi had a couple of examples of extracting methods. Start there.

As you move stuff to methods you will soon see that there are different categories of methods that are used together. This is an indication that these should be moved to a separate classes to encapsulate that behaviour.

Then read about the SOLID principles and continue from there. Practice, practice, practice and think about what you are doing at all times. Good luck!

\$\endgroup\$
  • \$\begingroup\$ +1 I love refactoring code when I've got unit tests written for it, I get to leverage as many layers of new or removed abstraction as I want without any worry of screwing up the logic. \$\endgroup\$ – Jimmy Hoffa Aug 17 '11 at 11:23

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.