I'm looking to refactor some code to make it more concise, and I'm a little stumped on one aspect. There's a good chance I may be missing something obvious.
The application is a webserver, where each API endpoint is a separate method of a RequestHandler class (Tornado specifically, but I'd like to know a general approach). So essentially, something like:
class MyHandler(tornado.web.RequestHandler):
def endpoint_1(self, user_id):
...
def endpoint_2(self, user_id):
...
Many of the endpoints need to lookup a user via the database, so I added a method like def get_user(self, user_id) to the class. This method also takes care of various permission checks required of the user, and as such there are several ways it can fail, and each failure should send an appropriate message back to the user and close the connection.
My question then is how best to call this method within each endpoint method in a way that is both concise and obvious.
I see three possible approaches:
- Catch all relevant exceptions within each endpoint method:
def endpoint_1(self, user_id):
try:
self.get_user(user_id)
except UserDoesNotExist:
self.write("No such user")
self.connection.close()
return
except MissingPermission1:
self.write("No permission 1")
self.connection.close()
return
...
This to me is the most clear as to control flow, but requires making sure these except: blocks are properly copied into each method.
- Unwrapped
get_user()call, letget_user()handle closing the connection
def get_user(self, user_id):
try:
user = self.database.lookup(user_id)
if not user.hasPermission(1):
raise MissingPermission1
...
except UserDoesNotExist:
self.write("No such user")
self.connection.close()
except MissingPermission1:
self.write("No permission 1")
self.connection.close()
...
def endpoint_1(self, user_id):
user = self.get_user(user_id)
...
This option is by far the most concise but something feels "dirty" to me about letting get_user() close the connection, preemptively ending the control flow of endpoint_1(). Maybe this is just me though? Documentation could solve this issue, but I still just get bad code smells from this.
- Catch-all except, pass to an exception-handling method
def get_user(self, user_id):
user = self.database.lookup(user_id)
if not user.hasPermission(1):
raise MissingPermission1
...
def handle_exception(self, exc):
if isinstance(exc, MissingPermission1):
self.write("Missing permission 1")
return
...
def endpoint_1(self, user_id):
try:
self.get_user(user_id)
except Exception as e:
self.handle_exception(e)
self.connection.close()
return
This is the approach I like the most, but something about it still feels off.
I'm very curious as to the what others think on this, preferably in as general as possible (e.g. not Tornado/webserver specific).