1
\$\begingroup\$

I had an efficiency problem like I thought and I didn't have the best solution here.

My solution was \$O(n)\$ and directly a more experienced member told me a solution that is \$O(1)\$, that only fetches one element and doesn't have to do a count. I'd like to rewrite to that solution, if you agree that is the next thing I should do. I've also been considering adding the function crop image to the Facebook application I write where there is no bug. I just plan to implement the proposed change so that images get fetches faster exactly like was suggested as answer to the question above.

Do you agree that it's the next thing to do about this code?

import random
from google.appengine.api import files, images


class CyberFazeHandler(BaseHandler):

    """
    Every time you call this function, it will perform a count operation, which is O(n) with the number of FileInfo entities, then perform an offset   query, which is O(n) with the offset. This is extremely slow and inefficient, and will get more so as you increase the number of images.

    If you expect the set of images to be small (less than a few thousand) and fairly constant, simply store them in code, which will be faster than any other option. If the set is larger, or changes at runtime, assign a random value between 0 and 1 to each entity, and use a query like this to retrieve a randomly selected one:
    """

    def get_random_image(self, category):
        fileinfos = FileInfo.all().filter('category =', category)
        return fileinfos[random.randint(0, fileinfos.count() - 1)]

    """ do like this instead
   q = FileInfo.all()
   q.filter('category =', category)
   q.filter('random >=', random.random())
   return q.get()
   """


    def get(self):


    """
    If the user will be loading a lot of these mashups, it makes more sense to send them as separate images, because there will be fewer images for the browser to cache (a+b+c images instead of a*b*c).
    """

        eyes_image = self.get_random_image(category='eyes')
        nose_image = self.get_random_image(category='nose')
        mouth_image = self.get_random_image(category='mouth')
        eyes_data = None
        try:
            eyes_data = blobstore.fetch_data(eyes_image.blob.key(), 0,
                    50000)
        except Exception, e:
            self.set_message(type=u'error',
                             content=u'Could not find eyes data for file '
                              + str(eyes_image.key().id()) + ' ('
                             + unicode(e) + u')')

        eyes_img = None

        try:
            eyes_img = images.Image(image_data=eyes_data)
        except Exception, e:
            self.set_message(type=u'error',
                             content=u'Could not find eyes img for file '
                              + str(eyes_image.key().id()) + ' ('
                             + unicode(e) + u')')

        nose_data = None
        try:
            nose_data = blobstore.fetch_data(nose_image.blob.key(), 0,
                    50000)
        except Exception, e:
            self.set_message(type=u'error',
                             content=u'Could not find nose data for file '
                              + str(nose_image.key().id()) + ' ('
                             + unicode(e) + u')')

        nose_img = None

        try:
            nose_img = images.Image(image_data=nose_data)
        except Exception, e:
            self.set_message(type=u'error',
                             content=u'Could not find nose img for file '
                              + str(nose_image.key().id()) + ' ('
                             + unicode(e) + u')')

        mouth_data = None
        try:
            mouth_data = blobstore.fetch_data(mouth_image.blob.key(),
                    0, 50000)
        except Exception, e:
            self.set_message(type=u'error',
                             content=u'Could not find mouth data for file '
                              + str(eyes_image.key().id()) + ' ('
                             + unicode(e) + u')')

        mouth_img = None

        try:
            mouth_img = images.Image(image_data=mouth_data)
        except Exception, e:
            self.set_message(type=u'error',
                             content=u'Could not find mouth img for file '
                              + str(mouth_image.key().id()) + ' ('
                             + unicode(e) + u')')

        minimum = min(int(eyes_img.width), int(nose_img.width),
                      int(mouth_img.width))

        eyes_url = images.get_serving_url(str(eyes_image.blob.key()),
                size=minimum)
        nose_url = images.get_serving_url(str(nose_image.blob.key()),
                size=minimum)
        mouth_url = images.get_serving_url(str(mouth_image.blob.key()),
                size=minimum)

        self.render(
            u'cyberfaze',
            minimum=minimum,
            eyes_image=eyes_image,
            eyes_url=eyes_url,
            nose_image=nose_image,
            nose_url=nose_url,
            mouth_image=mouth_image,
            mouth_url=mouth_url,
            form_url=blobstore.create_upload_url('/upload'),
            )


class UserRunsHandler(BaseHandler):

    """Show a specific user's runs,"""

                                      # ensure friendship with the logged in user"""

    @user_required
    def get(self, user_id):
        if True:  # self.user.friends.count(user_id) or self.user.user_id == user_id:
            user = User.get_by_key_name(user_id)
            if not user:
                self.set_message(type=u'error',
                                 content=u'That user does not use Run with Friends.'
                                 )
                self.redirect(u'/')
                return

            self.render(u'user', user=user,
                        runs=Run.find_by_user_ids([user_id]))
        else:
            self.set_message(type=u'error',
                             content=u'You are not allowed to see that.'
                             )
            self.redirect(u'/')


class RunHandler(BaseHandler):

    """Add a run"""

    @user_required
    def post(self):
        try:
            location = self.request.POST[u'location'].strip()
            if not location:
                raise RunException(u'Please specify a location.')

            distance = float(self.request.POST[u'distance'].strip())
            if distance < 0:
                raise RunException(u'Invalid distance.')

            date_year = int(self.request.POST[u'date_year'].strip())
            date_month = int(self.request.POST[u'date_month'].strip())
            date_day = int(self.request.POST[u'date_day'].strip())
            if date_year < 0 or date_month < 0 or date_day < 0:
                raise RunException(u'Invalid date.')
            date = datetime.date(date_year, date_month, date_day)

            run = Run(user_id=self.user.user_id, location=location,
                      distance=distance, date=date)
            run.put()

            title = run.pretty_distance + u' miles @' + location
            publish = u'<a onclick=\'publishRun(' \
                + json.dumps(htmlescape(title)) \
                + u')\'>Post to facebook.</a>'
            self.set_message(type=u'success',
                             content=u'Added your run. ' + publish)
        except RunException, e:
            self.set_message(type=u'error', content=unicode(e))
        except KeyError:
            self.set_message(type=u'error',
                             content=u'Please specify location, distance & date.'
                             )
        except ValueError:
            self.set_message(type=u'error',
                             content=u'Please specify a valid distance & date.'
                             )
        except Exception, e:
            self.set_message(type=u'error',
                             content=u'Unknown error occured. ('
                             + unicode(e) + u')')
        self.redirect(u'/')


class RealtimeHandler(BaseHandler):

    """Handles Facebook Real-time API interactions"""

    csrf_protect = False

    def get(self):
        if self.request.GET.get(u'setup') == u'1' and self.user \
            and conf.ADMIN_USER_IDS.count(self.user.user_id):
            self.setup_subscription()
            self.set_message(type=u'success',
                             content=u'Successfully setup Real-time subscription.'
                             )
        elif self.request.GET.get(u'hub.mode') == u'subscribe' \
            and self.request.GET.get(u'hub.verify_token') \
            == conf.FACEBOOK_REALTIME_VERIFY_TOKEN:

            self.response.out.write(self.request.GET.get(u'hub.challenge'
                                    ))
            logging.info(u'Successful Real-time subscription confirmation ping.'
                         )
            return
        else:
            self.set_message(type=u'error',
                             content=u'You are not allowed to do that.')
        self.redirect(u'/')

    def post(self):
        body = self.request.body
        if self.request.headers[u'X-Hub-Signature'] != u'sha1=' \
            + hmac.new(self.facebook.app_secret, msg=body,
                       digestmod=hashlib.sha1).hexdigest():
            logging.error(u'Real-time signature check failed: '
                          + unicode(self.request))
            return
        data = json.loads(body)

        if data[u'object'] == u'user':
            for entry in data[u'entry']:
                taskqueue.add(url=u'/task/refresh-user/' + entry[u'id'])
                logging.info('Added task to queue to refresh user data.'
                             )
        else:
            logging.warn(u'Unhandled Real-time ping: ' + body)

    def setup_subscription(self):
        path = u'/' + conf.FACEBOOK_APP_ID + u'/subscriptions'
        params = {
            u'access_token': conf.FACEBOOK_APP_ID + u'|' \
                + conf.FACEBOOK_APP_SECRET,
            u'object': u'user',
            u'fields': _USER_FIELDS,
            u'callback_url': conf.EXTERNAL_HREF + u'realtime',
            u'verify_token': conf.FACEBOOK_REALTIME_VERIFY_TOKEN,
            }
        response = self.facebook.api(path, params, u'POST')
        logging.info(u'Real-time setup API call response: '
                     + unicode(response))


class RefreshUserHandler(BaseHandler):

    """Used as an App Engine Task to refresh a single user's data if possible"""

    csrf_protect = False

    def post(self, user_id):
        logging.info('Refreshing user data for ' + user_id)
        user = User.get_by_key_name(user_id)
        if not user:
            return
        try:
            user.refresh_data()
        except FacebookApiError:
            user.dirty = True
            user.put()


class FileInfo(db.Model):

    blob = blobstore.BlobReferenceProperty(required=True)
    uploaded_by = db.UserProperty()
    facebook_user_id = db.StringProperty()
    uploaded_at = db.DateTimeProperty(required=True, auto_now_add=True)
    category = db.CategoryProperty(choices=('eyes', 'nose', 'mouth',
                                   'other'))


class FileBaseHandler(webapp.RequestHandler):

    def render_template(self, file, template_args):
        path = os.path.join(os.path.dirname(__file__), 'templates',
                            file)
        self.response.out.write(template.render(path, template_args))


class FileUploadFormHandler(FileBaseHandler):

  # @util.login_required
  # @user_required

    def get(self):

    # user = users.get_current_user()

        if True:  # user:  # signed in already

        # self.response.out.write('Hello <em>%s</em>! [<a href="%s">sign out</a>]' % (
         #   user.nickname(), users.create_logout_url(self.request.uri)))

            self.render_template('upload.html', {'logout_url'
                                 : (users.create_logout_url(r'/'
                                 ) if users.get_current_user() else None)})
        else:

              # let user choose authenticator

            self.response.out.write('Hello world! Sign in at: ')


class FileUploadHandler(BaseHandler,
    blobstore_handlers.BlobstoreUploadHandler):

    csrf_protect = False

    def post(self):
        blob_info = self.get_uploads()[0]
        if False:  # not users.get_current_user():
            blob_info.delete()
            self.redirect(users.create_login_url(r'/'))
            return

        file_info = FileInfo(blob=blob_info.key())  # ,
        logging.debug('if user')

        if self.user:
            logging.debug('found user')
            file_info.facebook_user_id = self.user.user_id
            logging.debug('set user id')

        db.put(file_info)
        self.redirect('/file/%d' % (file_info.key().id(), ))


class AjaxSuccessHandler(FileBaseHandler):

    def get(self, file_id):
        self.response.headers['Content-Type'] = 'text/plain'
        self.response.out.write('%s/file/%s' % (self.request.host_url,
                                file_id))


class FileInfoHandler(BaseHandler, FileBaseHandler):

    def get(self, file_id):
        file_info = FileInfo.get_by_id(long(file_id))
        if not file_info:
            self.error(404)
            return
        self.render(u'info', file_info=file_info,
                    logout_url=users.create_logout_url(r'/'))


class FileDownloadHandler(blobstore_handlers.BlobstoreDownloadHandler):

    def get(self, file_id):
        file_info = FileInfo.get_by_id(long(file_id))
        if not file_info or not file_info.blob:
            self.error(404)
            return
        self.send_blob(file_info.blob, save_as=True)


class GenerateUploadUrlHandler(FileBaseHandler):

  # @util.login_required

    def get(self):
        self.response.headers['Content-Type'] = 'text/plain'
        self.response.out.write(blobstore.create_upload_url('/upload'))


class ServeHandler(blobstore_handlers.BlobstoreDownloadHandler):

    def get(self, resource):
        resource = str(urllib.unquote(resource))
        blob_info = blobstore.BlobInfo.get(resource)
        self.send_blob(blob_info)


class SetCategoryHandler(webapp.RequestHandler):

    def get(self, file_id):
        file_info = FileInfo.get_by_id(long(file_id))
        if not file_info or not file_info.blob or file_id == '25001':
            self.error(404)
            return
        file_info.category = self.request.get('cg')
        file_info.put()
        self.response.out.write('category updated')


def main():
    routes = [
        (r'/', CyberFazeHandler),
        (r'/user/(.*)', UserRunsHandler),
        (r'/run', RunHandler),
        (r'/realtime', RealtimeHandler),
        (r'/task/refresh-user/(.*)', RefreshUserHandler),
        ('/ai', FileUploadFormHandler),
        ('/serve/([^/]+)?', ServeHandler),
        ('/upload', FileUploadHandler),
        ('/generate_upload_url', GenerateUploadUrlHandler),
        ('/file/([0-9]+)', FileInfoHandler),
        ('/file/set/([0-9]+)', SetCategoryHandler),
        ('/file/([0-9]+)/download', FileDownloadHandler),
        ('/file/([0-9]+)/success', AjaxSuccessHandler),
        ]

    application = webapp.WSGIApplication(routes,
            debug=os.environ.get('SERVER_SOFTWARE', '').startswith('Dev'
            ))
    util.run_wsgi_app(application)


if __name__ == u'__main__':
    main()
\$\endgroup\$
  • 1
    \$\begingroup\$ the next thing you do is up to you. There isn't a defined order of how you should do things. What are your priorities? That's how you decide where you should spend your effort. \$\endgroup\$ – Winston Ewert Sep 28 '11 at 21:07
  • \$\begingroup\$ @Winston Ewert I fixed this issue and now I just do next thing and it's up to me. Thank you for the excellent encouragement and support. I prioritize performance, security and user-friendliness in that order. Is there something else to prioritize? \$\endgroup\$ – Niklas Sep 29 '11 at 6:04
  • 2
    \$\begingroup\$ How about correctness? \$\endgroup\$ – Winston Ewert Sep 29 '11 at 12:07
  • \$\begingroup\$ Naturally and only the best solution is the correct one. \$\endgroup\$ – Niklas Sep 29 '11 at 15:48
  • 1
    \$\begingroup\$ Right, so I'd probably put my priorties as security, correctness, user-friendliness, and then performance. So performance is the least important concern. Its much more important that its correct, easy to use and secure then it being fast. \$\endgroup\$ – Winston Ewert Sep 29 '11 at 15:55
2
\$\begingroup\$
class CyberFazeHandler(BaseHandler):

    def get_random_image(self, category):

I'd recommend you consider adding docstrings, to a give quick explanation what your functions are doing.

        q = FileInfo.all()
        q.filter('category =', category)
        q.filter('randomvalue >=', random.random())
        return q.get()

    def get_random_image_legacy(self, category):
        fileinfos = FileInfo.all().filter('category =', category)
        return fileinfos[random.randint(0, fileinfos.count() - 1)]

Why would you keep a legacy method around?

    def get(self):

        eyes_image = self.get_random_image(category='eyes')
    if not eyes_image:
       logging.debug("getting eyes failed, trying legacy method")
           eyes_image = self.get_random_image_legacy(category='eyes')
        nose_image = self.get_random_image(category='nose')
    if not nose_image:
           nose_image = self.get_random_image_legacy(category='nose')

        mouth_image = self.get_random_image(category='mouth')
    if not mouth_image:
           mouth_image = self.get_random_image_legacy(category='mouth')

You've got pretty much the same thing repeated three times. Write a generic function that can handle getting the image and handling the failure case

        eyes_data = None
        try:
            eyes_data = blobstore.fetch_data(eyes_image.blob.key(), 0,
                    50000)
        except Exception, e:
            self.set_message(type=u'error',
                             content=u'Could not find eyes data for file '
                              + str(eyes_image.key().id()) + ' ('
                             + unicode(e) + u')')

Do you really want to catch any exception here? Usually you want to catch a more specific exception.

        eyes_img = None

Do this in the except clause

        try:
            eyes_img = images.Image(image_data=eyes_data)
        except Exception, e:
            self.set_message(type=u'error',
                             content=u'Could not find eyes img for file '
                              + str(eyes_image.key().id()) + ' ('
                             + unicode(e) + u')')

        nose_data = None
        try:
            nose_data = blobstore.fetch_data(nose_image.blob.key(), 0,
                    50000)
        except Exception, e:
            self.set_message(type=u'error',
                             content=u'Could not find nose data for file '
                              + str(nose_image.key().id()) + ' ('
                             + unicode(e) + u')')

        nose_img = None

        try:
            nose_img = images.Image(image_data=nose_data)
        except Exception, e:
            self.set_message(type=u'error',
                             content=u'Could not find nose img for file '
                              + str(nose_image.key().id()) + ' ('
                             + unicode(e) + u')')

        mouth_data = None
        try:
            mouth_data = blobstore.fetch_data(mouth_image.blob.key(),
                    0, 50000)
        except Exception, e:
            self.set_message(type=u'error',
                             content=u'Could not find mouth data for file '
                              + str(eyes_image.key().id()) + ' ('
                             + unicode(e) + u')')

        mouth_img = None

        try:
            mouth_img = images.Image(image_data=mouth_data)
        except Exception, e:
            self.set_message(type=u'error',
                             content=u'Could not find mouth img for file '
                              + str(mouth_image.key().id()) + ' ('
                             + unicode(e) + u')')

Again almost exact code duplicated several times, refactor it into a function.

        minimum = min(int(eyes_img.width), int(nose_img.width),
                      int(mouth_img.width))

I'd put the int on the outside so you don't have to repeat it

        eyes_url = images.get_serving_url(str(eyes_image.blob.key()),
                size=minimum)
        nose_url = images.get_serving_url(str(nose_image.blob.key()),
                size=minimum)
        mouth_url = images.get_serving_url(str(mouth_image.blob.key()),
                size=minimum)

        self.render(
            u'cyberfaze',
            minimum=minimum,
            eyes_image=eyes_image,
            eyes_url=eyes_url,
            nose_image=nose_image,
            nose_url=nose_url,
            mouth_image=mouth_image,
            mouth_url=mouth_url,
            form_url=blobstore.create_upload_url('/upload'),
            )

Basically, you've got the same logic repeating for eyes, nose and mouth. I'd probably write a FaceFeature object to handle that logic and then create three of them. That way we'd get rid of the duplication.

\$\endgroup\$
  • \$\begingroup\$ Thank you for very clear instructions how I can improve! The reason I kept the legacy method is that even though it is slow (it uses count()) it always works so if there was an error with the new code that is faster since it only fetches one element then it could work slower and with the legacy function. The change I did was to add the randomness to the entity instead of the fetch. \$\endgroup\$ – Niklas Sep 29 '11 at 20:03
  • \$\begingroup\$ @NiklasR, If there is an error with the new code fix that, don't introduce a fallback. \$\endgroup\$ – Winston Ewert Sep 29 '11 at 20:32

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.