patternpythonMinor
Tornado quiz app
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:
The following scheme is valid for submitting answers:
So, in summary:
-
User logs in via Facebook, his Facebook ID will be stored in cookie
-
He will presented with the homepage where his current score is displayed and link to next question by:
-
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)
-
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
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/scoreuser_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
-
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
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:
However, that's probably just because I'm a Flask user, so I'm in love with decorators.
Conciser using
BaseHandler
Having a function that returns
MainHandler
The name
An oft advised idiom on this site is the use of guard clauses. I'd recommend using that here; instead of
Something like this will read easier
See something like this for a better justification.
In
Q0Handler
Numbers in class names make me feel slightly sick, and I have no idea what
We've already discussed the
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 junkHowever, 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 asdef 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
urlencodeon theuser_idvariable before constructing a URL with it.
- You should check
r.status_codeto 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 adictcall. I would however check that it is indeed adictthat you are getting.
- My, you seem to love returning
Nonein place of raising an exception or something. I'd recommend against it; returning aNoneis 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 `usCode 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 junkdef 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.