HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

Add lister class

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
classlisteradd

Problem

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 twitt

Solution

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

Code Snippets

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

...

ad.email = self.value_of('email')
ad.email = self.request.POST.get(param, None)
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')
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);

Context

StackExchange Code Review Q#4164, answer score: 5

Revisions (0)

No revisions yet.