patternpythonMinor
Add lister class
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
```
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
structures that take a lot of space e.g.
or even better (as provided by James Khoury):
and
The second thing I would do is try to divide the long function to multiple functions that can be sensibly named. Possible functions include
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.
Disclaimer: I don't know Python
try:
...
except Exceptiopn:
passstructures 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:
passdef 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.