On a code review, I've stumbled on an interesting idea, which I can't properly judge alone.
Is it OK to improve readability of code by means of not widely known language syntax? When the original writer didn't use a special syntax designated for solving a specific coding problem, and solved it by simulating with workarounds — is it worth to refactor it for clarity and conciseness? What if even the code reviewer didn't know about the language feature?
To be specific, I'll bring the exact issue I've met on the code review — but keep in mind that this is just an example.
So the original code was a little web service in Python. At some point, there arised a requirement for several request handlers to be protected with an authentication mechanism. Somebody solved it with inheritance:
class AuthOnlyAccess:
def GET(self, *args):
if not hasattr(self, 'PROTECTED_GET'):
raise web.Nomethod()
if check_auth():
return self.PROTECTED_GET(*args)
else:
raise web.Unauthorised()
def POST(self, *args):
if not hasattr(self, 'PROTECTED_POST'):
raise web.Nomethod()
if check_auth():
return self.PROTECTED_POST(*args)
else:
raise web.Unauthorised()
...
class FooHandler(AuthOnlyAccess):
def PROTECTED_GET(self, a, b, c):
handle_foo_get(a, b, c)
def PROTECTED_POST(self, x, y, z):
handle_spam_eggs()
...
<more handlers deriving AuthOnlyAccess>
The refactorer found this code ugly and introduced a decorator:
from functools import wraps
...
def requires_auth(wrapped_func):
""" (a little doc telling that this is a decorator and how to use it) """
@wraps(wrapped_func)
def decorator(*args):
if check_auth():
return wrapped_func(*args)
else
raise web.Unauthorized()
return decorator
...
class FooHandler:
@requires_auth
def GET(self, a, b, c):
handle_foo_get(a, b, c)
@requires_auth
def POST(self, x, y, z):
handle_spam_eggs()
...
<more refactored handlers>
The reviewer argues that this change actually decreases maintainability, because most of the team doesn't know decorator syntax. So the next person visiting the refactored code could be puzzled by the construct — but not with the previous version.
There's also a convincing argument that most of the codebase isn't in Python, (the language is there only for that little web service), and hence team members aren't required to have even shallow knowledge of Python. However, to work with code, one's going to need to know its language, right?
And if you have the temptation to say that python decorators aren't that advanced syntax — please remember that this is only an example. Every language has its dark corners. (But to learn those corners means to become better.)
So the question is... Can you read unknown syntax? Is the elegance worth the surprise? At least, what are the tradeoffs and/or guidelines you can name? I do really need some opinions from the community of programming professionals.
