Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Please review this simple code. When I review it I decide to rewrite so that it doesn't write to a template, but instead directly to output since the whole HTML is fetched from the data layer. I used the tool appengine_admin to get editable HTML for simple html capabilities to an appspot project:

    class Page(db.Model):
      html = db.TextProperty(required=True)

    class AdminPage(appengine_admin.ModelAdmin):

        model = Page
        listFields = (
            'html',
            )
        editFields = (
            'html',
            )



  application = webapp.WSGIApplication([  ('/page/([0-9]+)', PageHandler), ])

  class PageHandler(BaseHandler):
      def get(self, file_id):
        page = Page.get_by_id(long(file_id))
        if not page:
          self.error(404)
          return
        self.render_template(file_id+'.html', {
            'body': page.body,
        })

My 2 to-dos:

  1. change variable file_id to page_id
  2. change writing to direct output instead of template
share|improve this question

1 Answer 1

up vote 3 down vote accepted
  application = webapp.WSGIApplication([  ('/page/([0-9]+)', PageHandler), ])

I'd put the list of urls as a global constant, I think that would make it easier to read.

  class PageHandler(BaseHandler):
      def get(self, file_id):
        page = Page.get_by_id(long(file_id))
        if not page:

If you are checking against None here, I'd use if page is None:

  self.error(404)
  return

I'd use else instead of returning here

self.render_template(file_id+'.html', {
    'body': page.body,
})
share|improve this answer
    
Thank you Winston –  Programmer 400 Oct 26 '11 at 6:21

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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