Code smells, fourth part

By Detlef Wuppermann

In our first three parts of our series we discussed a small (Python) script. We went through the most obvious code smells and fixed them in the code below.

from datetime import datetime, timedelta
import sys

import requests
from requests.auth import HTTPBasicAuth

from configuration import http_user_service_url, http_user_service_username, http_user_service_password,
                          userdata_filename, age_in_years

import csv

authentication = None
min_age = None
file_handler = None
writer = None

def setup():
    global authentication
    global min_age
    global writer
    global file_handler

    authentication = HTTPBasicAuth(http_user_service_username, http_user_service_password)

    min_age = datetime.now() - timedelta(days=age_in_years * 365)

    file_handler = open(userdata_filename, 'w')
    writer = csv.writer(file_handler)

def tear_down():
    file_handler.close()

def is_user_from_country(user):
    if user['country'] == "Spain":
        return True
    LOG.debug(f"User with id ${user['id']} is not from Spain.")
    return False

def is_user_old_enough(user):
    global min_age

    birth_date_str = user['birth_date']
    birth_date = datetime.fromisoformat(birth_date_str)
    is_old_enough = birth_date < min_age
    if not is_old_enough:
        LOG.debug(f"User with id ${user['id']} is not old enough and will be skipped.")
    return is_old_enough

def get_all_users():
    global authentication

    LOG.debug(f"Fetching data from user service with response code {response.code}.")
    response = requests.get(http_user_service_url, auth=authentication)

    if response.status_code != 200:
        LOG.error(f"Could not make a successful request to ${http_user_service_url}. The error message is {response.text}.")
        sys.exit(1)

    response_dict = response.json()
    all_users = response_dict['results']

def write_user(user, user_count):
    new_user = {
        "name": f"{user["title"]} {user["first_name"]} {user["last_name"]}",
        "street": user["street"],
        "city": f"{user["postal_code"]} {user["city"]}",
        "country": user["country"],
    }
    writer.writerow(user.keys())

def filter_users_of_legal_drinking_age():
    all_users = get_all_users()
    filtered_users_count = 0
    for user in all_users:
        if not is_old_enough(user):
            continue
        if not is_user_from_country(user):
            continue
       write_user(user, filtered_users_count)
       filtered_users_count = filtered_users_count + 1
    LOG.debug(f"{filtered_users_count} from {len(all_users)} total users found.")


if __main__ == main():
    setup()
    filter_users_of_legal_drinking_age()
    tear_down()

We reorganized our code and put everything in dedicated functions that explain better what they do. As one example, we created the function filter_users_of_legal_drinking_age that now explains what we are really doing, namely extracting all the users that are allowed to have a beer or two.

We could have explained it with comments, but now we do it with code, resp. the name of the function.

This function also is a gist of what the whole script is doing, fetching all users, iterating through them, and writing those users that fit two requirements. We removed all the technical information in this function, i.e., how we get the user (in this example from a REST endpoint) and where we write the information to (in this case to a file). Imagine you had to explain what your application does to your boyfriend (or girlfriend for that matter). You could read the code out loud, and it should be clear what happens.

We also changed the way we filtered for the user. Instead of checking if one user fulfills a requirement and continue with the next check, we check if the user doesn’t fulfill it so that we can exit the nested if-loops as soon as possible. When reading this we do not have to remember what exactly all the if statements do, we jump into the functions once we are interested in the inner working of it.

Now, imagine you had to filter also for other properties of a user. Implementing it in the existing code would be easy. Just create a new function and filter for it in the filter_users_of_legal_drinking_age function. Adding such a function would not affect the existing code and would not mess up the existing code or rather the readability.

If we had a bug in the code, we did not have to go through all the code searching for the place where we could have the bug. It is now structured, and we have a function with a name that explains what it is doing exactly.