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):
message = ''
challenge,
response,
remoteip)
if cResponse.is_valid==True:
isHuman=True
else:#failed anti-spam test and can try again
isHuman=False
#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(),}
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
import random
twenty = random.randint(1,5) > 3
data = memcache.get(memcache_key)
if data is not None:
return data
geoipcode = ''
try:
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:
except Exception:

import util
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:
logo = 'montao'

if (util.get_host().find('acktuellt') > 0 or twenty):
#path = os.path.join(os.path.dirnam e(__file__), 'market', 'market_ad_preview.html')
if (not twenty or util.get_host().find('acktuellt') > 0):
path = os.path.join(os.path.dirname(__file__), 'market', 'credit.html')
if users.get_current_user():
try:
except Exception:
pass
try:
except Exception:
pass
try:
except Exception:
pass
try:
except Exception, ex:
logging.debug("failed creating facebookID object for user %s", str(ex))
pass

try:
info = client.get('/account/verify_credentials')
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.geohash = Geohash.encode(float(lat),float(lng), precision=2)#evalu8 precision variable
except Exception:
pass
try:
except Exception:
logging.error(Exception)
try:
except Exception:
pass
try:
ad.phoneview = self.request.get('phone_hidden',None) is not None
except Exception:
pass
try:
except Exception:
pass
try:
except Exception:
pass
try:
except Exception:
pass
try:
except Exception:
pass
self.s = session.Session()
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')
logging.debug('creating image')
img.primary_image = blob_key
logging.debug('creating image')
img.put()
except Exception:
self.response.write(Exception)
#pass
#deprecate see http://stackoverflow.com/questions/5863305/storing-mime-type-with-blobstore
try:
filedata = file
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:
except Exception:
pass
try:
except Exception:
pass
try:
except Exception:
pass
try:
except Exception:
pass
try:
except Exception:
pass
try:
except Exception:
size = 9
vowels='aeiou'
consonants='bcdfghjklmnpqrstvwxyz'
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)
vowel = vowels[random.randint(1,len(vowels)-1)]
if random.choice([1,0]):
vowel=string.upper(vowel)
try:
im_from = db.IM("xmpp", 'classifiedsmarket.appspot.com')
except Exception:
pass


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

...



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:
except Exception:
pass

...



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

• is_submitter_human
• 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()


Disclaimer: I don't know Python

• 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. – Niklas R. Aug 17 '11 at 19:52
• 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? – James Khoury Aug 18 '11 at 23:25
• In my defense: I did write a disclaimer. – Aleksi Yrttiaho Aug 19 '11 at 9:06
• Excellent review thanks to Aleksi and James. I needed this. :-) – Niklas R. Aug 20 '11 at 13:52

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:


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!

• +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. – Jimmy Hoffa Aug 17 '11 at 11:23