On Github treyhunner / readability-counts
Hey everybody! 😄
So let's talk about readability.
Let's take a look at an example
employee_hours = (schedule.earliest_hour for employee in self.public_employees for schedule in employee.schedules) return min(h for h in employee_hours if h is not None)
employee_hours = ( schedule.earliest_hour for employee in self.public_employees for schedule in employee.schedules ) return min( hour for hour in employee_hours if hour is not None )
Let's talk about regular expressions
def is_valid_uuid(uuid): """Return True if given variable is a valid UUID.""" return bool(re.search(r'^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}' r'-[0-9a-f]{4}-[0-9a-f]{12}$', uuid, re.IGNORECASE))
this is a function that uses a regular expression to validate the string as a UUID
is this code readable?
this code is difficult to read because regular expressions are information dense
So regular expressions are code. They're like mini programs that are written all on line, without whitespace or comments
def is_valid_uuid(uuid): """Return True if given variable is a valid UUID.""" UUID_RE = re.compile(r''' ^ [0-9a-f] {8} # 8 hex digits - [0-9a-f] {4} # 4 hex digits - [0-9a-f] {4} # 4 hex digits - [0-9a-f] {4} # 4 hex digits - [0-9a-f] {12} # 12 hex digits $ ''') return bool(UUID_RE.search(uuid, re.IGNORECASE | re.VERBOSE))
default_appointment = models.ForeignKey( 'AppointmentType', null=True, on_delete=models.SET_NULL, related_name='+')
default_appointment = models.ForeignKey('AppointmentType', null=True, on_delete=models.SET_NULL, related_name='+')
default_appointment = models.ForeignKey( 'AppointmentType', null=True, on_delete=models.SET_NULL, related_name='+')
default_appointment = models.ForeignKey( othermodel='AppointmentType', null=True, on_delete=models.SET_NULL, related_name='+')
default_appointment = models.ForeignKey(othermodel='AppointmentType', null=True, on_delete=models.SET_NULL, related_name='+')
default_appointment = models.ForeignKey( othermodel='AppointmentType', null=True, on_delete=models.SET_NULL, related_name='+')
default_appointment = models.ForeignKey( othermodel='AppointmentType', null=True, on_delete=models.SET_NULL, related_name='+')
default_appointment = models.ForeignKey( othermodel='AppointmentType', null=True, on_delete=models.SET_NULL, related_name='+' )
default_appointment = models.ForeignKey( othermodel='AppointmentType', null=True, on_delete=models.SET_NULL, related_name='+', )
Let's take a look at some code with poor variable names
sc = {} for i in csv_data: sc[i[0]] = i[1]
state_capitals = {} for i in capitals_csv_data: state_capitals[i[0]] = i[1]
state_capitals = {} for s, c, *_ in capitals_csv_data: state_capitals[s] = c
state_capitals = {} for state, capital, *_ in capitals_csv_data: state_capitals[state] = capital
Let's look at an example of code that could use some more variables
def detect_anagrams(word, candidates): anagrams = [] for candidate in candidates: if (sorted(word.upper()) == sorted(candidate.upper()) and word.upper() != candidate.upper()): anagrams.append(candidate)
def detect_anagrams(word, candidates): anagrams = [] for candidate in candidates: if is_anagram(word, candidate): anagrams.append(candidate)
def is_anagram(word1, word2): return (sorted(word1.upper()) == sorted(word2.upper()) and word1.upper() != word2.upper())
def is_anagram(word1, word2): word1, word2 = word1.upper(), word2.upper() return sorted(word1) == sorted(word2) and word1 != word2
def is_anagram(word1, word2): word1, word2 = word1.upper(), word2.upper() return ( sorted(word1) == sorted(word2) # words have same letters and word1 != word2 # words are not the same )
def is_anagram(word1, word2): word1, word2 = word1.upper(), word2.upper() are_different_words = (word1 != word2) have_same_letters = (sorted(word1) == sorted(word2)) return have_same_letters and are_different_words
def detect_anagrams(word, candidates): anagrams = [] for candidate in candidates: if is_anagram(word, candidate): anagrams.append(candidate) def is_anagram(word1, word2): word1, word2 = word1.upper(), word2.upper() are_different_words = word1 != word2 have_same_letters = sorted(word1) == sorted(word2) return have_same_letters and are_different_words
Let's take look at a complex Django model method
def update_appointment_types(self): """Delete/make appt. types and set default appt. type""" self.appt_types.exclude(specialty=self.specialty).delete() new_types = self.specialty.appt_types.exclude(agent=self) self.appt_types.bulk_create( AppointmentType(agent=self, appointment_type=type_) for type_ in new_types ) old_default_id = self.default_appt_id self.default_appt_type = self.specialty.default_appt_type if self.default_appt_type.id != old_default_id: self.save(update_fields=['default_appt_type'])
def update_appointment_types(self): """Delete/make appt. types and set default appt. type""" self.appt_types.exclude(specialty=self.specialty).delete() new_types = self.specialty.appt_types.exclude(agent=self) self.appt_types.bulk_create( AppointmentType(agent=self, appointment_type=type_) for type_ in new_types ) old_default_id = self.default_appt_id self.default_appt_type = self.specialty.default_appt_type if self.default_appt_type.id != old_default_id: self.save(update_fields=['default_appt_type'])
This code was broken into three sections because each of these section performs a different task
Let's add some comments to these sections
def update_appointment_types(self): """Delete/make appt. types and set default appt. type""" # Delete appointment types for specialty besides current one self.appt_types.exclude(specialty=self.specialty).delete() # Create new appointment types based on specialty (if needed) new_types = self.specialty.appt_types.exclude(agent=self) self.appt_types.bulk_create( AppointmentType(agent=self, appointment_type=type_) for type_ in new_types ) # Set default appointment type based on specialty old_default_id = self.default_appt_id self.default_appt_type = self.specialty.default_appt_type if self.default_appt_type.id != old_default_id: self.save(update_fields=['default_appt_type'])
def update_appointment_types(self): """Delete/make appt. types and set default appt. type""" # Delete appointment types for specialty besides current one self.appt_types.exclude(specialty=self.specialty).delete() # Create new appointment types based on specialty (if needed) new_types = self.specialty.appt_types.exclude(agent=self) self.appt_types.bulk_create( AppointmentType(agent=self, appointment_type=type_) for type_ in new_types ) # Set default appointment type based on specialty old_default_id = self.default_appt_id self.default_appt_type = self.specialty.default_appt_type if self.default_appt_type.id != old_default_id: self.save(update_fields=['default_appt_type'])
def update_appointment_types(self): """Delete/make appt. types and set default appt. type""" # Delete appointment types for specialty besides current one self.appt_types.exclude(specialty=self.specialty).delete() # Create new appointment types based on specialty (if needed) new_types = self.specialty.appt_types.exclude(agent=self) self.appt_types.bulk_create( AppointmentType(agent=self, appointment_type=type_) for type_ in new_types ) # Set default appointment type based on specialty old_default_id = self.default_appt_id self.default_appt_type = self.specialty.default_appt_type if self.default_appt_type.id != old_default_id: self.save(update_fields=['default_appt_type'])
def _delete_stale_appointment_types(self): """Delete appointment types for specialties besides ours""" self.appt_types.exclude(specialty=self.specialty).delete() def _create_new_appointment_types(self): """Create new appointment types based on specialty if needed""" new_types = self.specialty.appt_types.exclude(agent=self) self.appt_types.bulk_create( AppointmentType(agent=self, appointment_type=type_) for type_ in new_types ) def _update_default_appointment_type(self): """Set default appointment type based on specialty""" old_default_id = self.default_appt_id self.default_appt_type = self.specialty.default_appt_type if self.default_appt_type.id != old_default_id: self.save(update_fields=['default_appt_type'])
def update_appointment_types(self): """Delete/make appt. types and set default appt. type""" self._delete_stale_appointment_types() self._create_new_appointment_types() self._update_default_appointment_type()
Let's take a look at exception handling
db = DBConnection("mydb") try: records = db.query_all() finally: db.close()
class connect: def __init__(self, path): self.connection = DBConnection(path) def __enter__(self): return self.connection def __exit__(self): self.close()
with connect("mydb") as db: db.query_all()
from contextlib import closing with closing(DBConnection("mydb")) as db: db.query_all()
Let's talk about for loops
employees = [] for calendar, availabilities in calendar_availabilities: if availabilities: employees.append(calendar.employee)
This code loops over something
You can tell that even though it's blurred out.
This code actually does a little more than that though...
employees = [] for calendar, availabilities in calendar_availabilities: if availabilities: employees.append(calendar.employee)
Specifically: the purpose of this code is to
loop over something check a condition and create a new list from items that pass that conditionWe're using a list append, an if statement, and a for loop to accomplish this task.
There's a better way to write this code.
employees = [ calendar.employee for (calendar, availabilities) in calendar_availabilities if availabilities ]
Let's say we're creating a class to represent items in a shopping cart.
class ShoppingCart: def contains(self, product): """Return True if cart contains the product.""" def add(self, product, quantity): """Add the quantity of a product to the cart.""" def remove(self, product): """Completely remove a product from the cart.""" def set(self, product, quantity): """Set the quantity of a product in the cart.""" @property def count(self): """Return product count in cart, ignoring quantities.""" @property def is_empty(self): """Return True if cart is empty."""
class ShoppingCart: def __contains__(self, product): """Return True if cart contains the product.""" def __setitem__(self, product, quantity): """Set the quantity of a product in the cart.""" def __delitem__(self, product): """Completely remove a product from the cart.""" def __len__(self): """Return product count in cart, ignoring quantities.""" def __bool__(self): """Return True if cart is non-empty."""
If you're ever planning to make your own type of container, check out the helpers in the collections library first.
Let's talk about functions.
def get_connection(host, username, password): """Initialize IMAP server and login""" server = IMAP4_SSL(host) server.login(username, password) server.select("inbox") return server def close_connection(server): server.close() server.logout() def get_message_uids(server): """Return unique identifiers for each message""" return server.uid("search", None, "ALL")[1][0].split() def get_message(server, uid): """Get email message identified by given UID""" result, data = server.uid("fetch", uid, "(RFC822)") (_, message_text), _ = data message = Parser().parsestr(message_text) return message
This code connects to an IMAP server and reads email
def get_connection(host, username, password): """Initialize IMAP server and login""" server = IMAP4_SSL(host) server.login(username, password) server.select("inbox") return server def close_connection(server): server.close() server.logout() def get_message_uids(server): """Return unique identifiers for each message""" return server.uid("search", None, "ALL")[1][0].split() def get_message(server, uid): """Get email message identified by given UID""" result, data = server.uid("fetch", uid, "(RFC822)") (_, message_text), _ = data message = Parser().parsestr(message_text) return message
Notice that one of these functions returns a server object and the other three functions each accept a server object.
This should be a hint that something weird is going on.
If you ever find that you're repeatedly passing the same data to multiple functions, think of making a class.
class IMAPChecker: def __init__(self, host): """Initialize IMAP email server with given host""" self.server = IMAP4_SSL(host) def authenticate(self, username, password): """Authenticate with email server""" self.server.login(username, password) self.server.select("inbox") def quit(self): self.server.close() self.server.logout() def get_message_uids(self): """Return unique identifiers for each message""" return self.server.uid("search", None, "ALL")[1][0].split() def get_message(self, uid): """Get email message identified by given UID""" result, data = self.server.uid("fetch", uid, "(RFC822)") (_, message_text), _ = data message = Parser().parsestr(message_text) return message
This is exactly what classes were designed for.
Classes bundle functionality and data together.
@treyhunner http://WeeklyPython.Chat