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

Tornado quiz app

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

Problem

I am writing a simple quiz app using Tornado and MongoDB. The user authentication is done using Facebook and each user is uniquely identified by their Facebook ID. When a user first creates an account, his score will be set to zero and he can access only questions whose ID is less than or equal to his current score. And question IF start from zero.

The questions are accessed with URL and answers also submitted using URL only. Following is the URL scheme for valid for accessing questions:

myapp.com/q_id 
myapp.com/1
myapp.com/1/


The following scheme is valid for submitting answers:

myapp.com/q_id/answer
myapp.com/1/hi
myapp.com/1/hi/


So, in summary:

-
User logs in via Facebook, his Facebook ID will be stored in cookie

self.set_secure_cookie('user_id', str(user['id']))


-
He will presented with the homepage where his current score is displayed and link to next question by: myapp.com/score

user_id = self.get_secure_cookie('user_id')       
if user_id:
    users = self.application.db.users
    search_user = users.find_one({'user_id': user_id})
    if not search_user: #i.e. new user
        score = 0
        name = self._get_fb_name(user_id)
        users.save({'user_id': user_id, 
                    'score': score,
                    'name' : name
        })               
    else:
        score = search_user.get('score')
        name = search_user.get('name')


-
The user can access only questions whose ID equal to his current score (i.e. his next challenge) or less than current score (i.e. previously correctly answered questions)

current_score = int(self.get_secure_cookie('score'))
if int(q_id) > current_score:
    self.write('You cannot access this question as your score is less')
    return


-
User submits answers as explained in above URL schemes. If answer is correct, then his score is updated in DB.

```
question_data = questions.find_one({'q_id': q_id})
if not question_data:
self.write('Huh, this ques

Solution

I'm not a Tornado expert, so these comments will be general to Python web development.

I'll first answer your specific questions

-
I don't know about your regexp, but you have a serious problem with Q0Handler.get; when answer = None (as is the default parameter), the line answer = answer[1:] will fail. I don't know if the code never gets called with the default parameter, in which case you don't need the default, but otherwise, you should probabaly change this to something like answer = answer[1:] if answer is not None or None.

-
I would recommend checking the DB. In general, I like to think of all user data coming under 3 categories: Untrusted, Trusted but not reliable, Trusted and reliable. In web development, we can think of cookies as Untrusted (i.e. the user may manipulate they at will), secure/encryped cookies as Trusted but not reliable (i.e. the user may not manipulate the contents without it being obvious to you, but it's not guaranteed to {be,stay} {there,well formed}), and the database can be trusted (i.e. if I put data in there, it should stay there, though I note you are using MongoDB, so that invariant may not be applicable here).

When it comes to implementing those properties, I would say this: you need to handle the case where someone deletes a cookie (i.e. wrap self.get_secure_cookie('score') in a try catch or test for 'score''s existence before hand). You also need to deal with cases where people are logged in on two browsers; if you just get data from cookies, then these will have different scores, which could be frustrating for the user. So all in all, It would just be easier to use the database in most places.

I'll give some general comments now

Application

You currently hard code your urls, which could lead to a very brittle structure, and make extensibility a pain. Have you considered using class decorators to register route handlers? They'd look something like this:

class Application(tornado.web.Application):
     handlers = []

     @staticmethod
     def register_route(route):
         def inner(handler_class):
             Application.handlers.append((route, handler_class))
             return handler_class
         return inner

      ... # The rest of your Application class

 @Application.register_route(r'/foobar/(\d+)')
 class MyFoobarHandler(tornado.web.RequestHandler):
     .... # Standard handler junk


However, that's probably just because I'm a Flask user, so I'm in love with decorators.

Conciser using super(Application, self).__init__(...) over explicitly calling the constructor of the super class (this may not be possible, due to complications with old style classes, though don't quote me on that).

"mydb" wins the word for the worst name for a database I've seen all year ;)

BaseHandler

Having a function that returns True or None is usually a code smell. If you want a function that returns True and some value indicating the converse of True, usually people use False. In that case, you can rewrite the function as

def get_current_user(self):
    return bool(self.get_secure_cookie("user_name"))


MainHandler

The name MainHandler tells me nothing. Due to your routes being defined in a different place, maybe naming it something like IndexHandler would be better (though I still don't like that).

An oft advised idiom on this site is the use of guard clauses. I'd recommend using that here; instead of

if user_id:
    ...
    10 lines of code and 3 indentation levels ommited
    ...
    self.render("index.html")
else:
    self.render("login.html")


Something like this will read easier

if not user_id:
    self.render("login.html")
    return
 # Insert your 10 lines of code and many indentation levels here
 ...
 self.render("index.html")


See something like this for a better justification.

In _get_fb_name, some things to note:

  • If you were feeling REALLY paranoid (it's the best way to be), you should call urlencode on the user_id variable before constructing a URL with it.



  • You should check r.status_code to make sure the request succeeded.



  • If the request is well formed, r.json() will return a python dict (assuming the json has an object on the top level); no need to wrap it in a dict call. I would however check that it is indeed a dict that you are getting.



  • My, you seem to love returning None in place of raising an exception or something. I'd recommend against it; returning a None is easy to overlook, and exceptions give you a glorious traceback.



Q0Handler

Numbers in class names make me feel slightly sick, and I have no idea what Q0 means; question 0? Because this view seems to handle all questions, not just question 0.

We've already discussed the answer = answer[1:] bug. There are a couple of other bugs; if I delete my score cookie, I'll end up causing a bug when you try to do int((self.get_secure_cookie('score')); int(None) -> TypeError (same problem exists for `us

Code Snippets

class Application(tornado.web.Application):
     handlers = []

     @staticmethod
     def register_route(route):
         def inner(handler_class):
             Application.handlers.append((route, handler_class))
             return handler_class
         return inner

      ... # The rest of your Application class

 @Application.register_route(r'/foobar/(\d+)')
 class MyFoobarHandler(tornado.web.RequestHandler):
     .... # Standard handler junk
def get_current_user(self):
    return bool(self.get_secure_cookie("user_name"))
if user_id:
    ...
    10 lines of code and 3 indentation levels ommited
    ...
    self.render("index.html")
else:
    self.render("login.html")
if not user_id:
    self.render("login.html")
    return
 # Insert your 10 lines of code and many indentation levels here
 ...
 self.render("index.html")

Context

StackExchange Code Review Q#42158, answer score: 2

Revisions (0)

No revisions yet.