Do you have a lot of short, single-use, private functions in your Python code?
Do you have a lot of short, single-use, private functions in your Python code? For example, below is some stubbed out authentication code I've been working on. It checks if a user's password is correct and updates the hash algorithm to use bcrypt. The 4 private functions with the leading underscore are from 1 to 10 lines long and are only used by the check_password function. These functions are part of a larger module with about 20 functions. I don't like that these 4 functions add clutter to the module and are not grouped with the function that uses them, check_password.
def _get_password_hash_from_db(email_address):
"""Get the user's password hash from the database.
"""
def _determine_password_hash_algorithm(password_hash):
"""Determine the hash algorithm.
"""
def _hash_password_old(password):
"""This is the OLD password hash algorithm.
"""
def _hash_existing_password_bcrypt(password, db_password_hash):
"""This is the NEW algorithm used for hashing existing passwords.
"""
def check_password(email_address, password):
"""Check if a user's supplied password is correct.
"""
db_password_hash = _get_password_hash_from_db(email_address)
hash_alg = _determine_password_hash_algorithm(db_password_hash)
if hash_alg == 'BCRYPT':
input_password_hash = _hash_existing_password_bcrypt(password, db_password_hash)
else:
input_password_hash = _hash_password_old(password)
password_correct = (input_password_hash == db_password_hash)
if password_correct and hash_alg != 'BCRYPT':
call_change_password(email_address, password)
return password_correct
def call_change_password(email_address, new_password):
"""Change the user's password.
"""
Sometimes, in cases like this, I move the 4 private functions to be nested functions inside check_password. I like how the functions are grouped together and that the module is not littered with extraneous functions. However, the inner functions are not easily testable and I don't see many people doing this.
def check_password(email_address, password):
"""Check if a user's supplied password is correct.
"""
def get_password_hash_from_db(email_address):
"""Get the user's password hash from the database.
"""
def determine_password_hash_algorithm(password_hash):
"""Determine the hash algorithm.
"""
def hash_password_old(password):
"""This is the OLD password hash algorithm.
"""
def hash_existing_password_bcrypt(password, db_password_hash):
"""This is the NEW algorithm used for hashing existing passwords.
"""
db_password_hash = get_password_hash_from_db(email_address)
hash_alg = determine_password_hash_algorithm(db_password_hash)
if hash_alg == 'BCRYPT':
input_password_hash = hash_existing_password_bcrypt(password, db_password_hash)
else:
input_password_hash = hash_password_old(password)
password_correct = (input_password_hash == db_password_hash)
if password_correct and hash_alg != 'BCRYPT':
call_change_password(email_address, password)
return password_correct
def call_change_password(email_address, new_password):
"""Change the user's password.
"""
Another option is to create a PasswordChecker class instead. This seems the most powerful and now the private methods are testable. However, this adds more overhead and I hear Jack Diederich telling me to Stop Writing Classes!
class _PasswordChecker(object):
"""Check if a user's supplied password is correct.
"""
@staticmethod
def _get_password_hash_from_db(email_address):
"""Get the user's password hash from the database.
"""
@staticmethod
def _determine_password_hash_algorithm(password_hash):
"""Determine the hash algorithm.
"""
@staticmethod
def _hash_password_old(password):
"""This is the OLD password hash algorithm.
"""
@staticmethod
def _hash_existing_password_bcrypt(password, db_password_hash):
"""This is the NEW algorithm used for hashing existing passwords.
"""
def __call__(self, email_address, password):
db_password_hash = self._get_password_hash_from_db(email_address)
hash_alg = self._determine_password_hash_algorithm(db_password_hash)
if hash_alg == 'BCRYPT':
input_password_hash = self._hash_existing_password_bcrypt(password, db_password_hash)
else:
input_password_hash = self._hash_password_old(password)
password_correct = (input_password_hash == db_password_hash)
if password_correct and hash_alg != 'BCRYPT':
call_change_password(email_address, password)
return password_correct
check_password = _PasswordChecker()
def call_change_password(email_address, new_password):
"""Change the user's password.
"""
Maybe the solution is to break up the module into smaller modules which act like the class above? However this might leave me with some unevenly sized modules. How do you handle this?
