Code smells, first part

Below you will find an example for a small script that we want to discuss. We want to take a closer look and ask ourselves how it can be improved. The idea behind this script is to make a request to a server, fetch a list of customers and filter for specific users (in this case users from Spain that are of a minimum certain age). After that we send the data to a server.

import datetime
from datetime import datetime, timedelta
import sys

# import requests module
import requests
from requests.auth import HTTPBasicAuth

import mysql.connector

import csv

# open the file in the write mode
f = open('./clients.csv', 'w')
# create the csv writer
writer = csv.writer(f)

# dataBase = mysql.connector.connect(
#     host ="localhost",
#     user ="u938453",
#     passwd ="Wxc6$78gfsgOi8kr"
# )

# print(dataBase)
# cursorObject = dataBase.cursor()

now = datetime.now()
min_age = now - timedelta(days=18 * 365)
# Making a get request
url = 'https://api.coding-on-my-mind.com/users'
response = requests.get(url, auth=HTTPBasicAuth('u938453', 'Wxc6$78gfsgOi8kr'))

# print(response)

if response.status_code != 200:
print(f"Could not make a successful request to ${url}")
sys.exit(1)

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

counter = 0
for user in all_users:
birth_date_str = user['birth_date']
birth_date = datetime.fromisoformat(birth_date_str)
if birth_date < min_age:


        if user['country'] == "Spain":

            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"],
            }
            if counter == 0:
                writer.writerow(new_user.keys())

            # write a row to the csv file
            writer.writerow(new_user.values())
            counter = counter + 1

            # sql = "insert into clients (title, first_name, last_name, street, postal_code, postal_code, city, country) VALUES (%s, %s, %s, %s, %s, %s, %s)"
            # val = new_user.values()
            # cursorObject.execute(sql, val)
            # dataBase.commit()
        else:
            print(f"User with id ${user['id']} is not from Spain")
    else:
        print(f"User with id ${user['id']} is not old enough and will be skipped")

# close the file
f.close()

# Disconnecting from the server
# dataBase.close()

So, my first question would be, is this even bad code? Well, the answer is as always: it depends. I wrote quite some scripts like this that were meant for small tasks only. I never shared this code and I updated the code as I needed to.

But what if this script is used by multiple colleagues? Then, in my opinion, it is only a matter of time until such code gets difficult to understand and maintain, and we want to improve our code.

For this, let’s take a closer look how we can improve it, step by step.

In our first part, we want to discuss some things that will be fixed in the code that you can find in our consecutive parts, where we improve our code further.

If you want to follow the examples you might want to copy this code to your own IDE of text editor, whatever you prefer. Take a minute and think of what you would have done different.

Let’s start with the most obvious code smells:

Spaghetti code

The most obvious in my opinion is that everything that we do is written without any structure. This makes the script harder to read. Although we only have some 80 lines of code in this case, I saw code that was much larger than that, but as unstructured as this one. That makes it really difficult to maintain the code.

Unused code

The main question that arises when I see unused code is: Is this code still needed four our use case? Do I have to care? Or ist this code obsolete and can be deleted?

Often, people are hesitant to delete code that once worked, so dead code is often still present in such scripts. My first thing that I do when I have to refactor code is to delete obsolete code. So why is this valid? Well, I always use a versioning system where I can look up older (working) versions of the code. Like this, old code does not pollute the actual code and does not make other developers think what this code could mean.

But what if this is code may be needed in the future? Then I still delete it. If we are sure that this functionality is still needed, why is it commented out? Is it commented out because it is not tested yet? If this possibility exists, I also want to get rid of it. Or how can I decide if this code does what it should do when I don’t know what it eventually should do?

But what I do is I will open a ticket with the possible requirements, or to be precise, a ticket that forces us to think about the importance of this (possible) functionality. We then still can decide if we want to have this functionality in the future or not (maybe we want, but we have way more important things to do before we can take care of this functionality).

Comments that explain the wrong things

In the example we can see comments that explain what happens in the following line(s) of code. But why? If I open a file I don’t have to explain it, if the next line is the command of opening a file. So, instead you want to explain why you do things.

Hard coded credentials, URLs and other values

Imagine, this code is in a code versioning system like Git. Then it is likely that this code can be read by multiple persons (basically everybody that has access to this repository).

Or you want to send this code to a colleague via email. These credentials could be personalized credentials that you don’t want to share with others.

Instead, you can define these values either in a file that you don’t share with others, or you can set them as environment variables.

Avoid unexplained numbers (so-called “magic numbers) in your code, replace them with named constants or variables that make the intention of your code clear.

In our second part we will do both, just to show you how it can be done. We will read the passwords from environment variables, the other configuration from a file.