Blog

Bad code and code smells examples in raiseexception

Most of the time when talking about software quality, you will find that the goal is to write loosely coupled and highly cohesive code. Although it sounds easy, it's really, really hard to achieve it.

Today I'm going to show you some bad code and code smells in RaiseException that make the code to have high coupling and/or low cohesion.

Concepts

Let's start with the meaning of coupling and cohesion. Take a look of these definitions taken from Grady Booch's book, Object Oriented Analysis and Design with Applications 3rd Edition:

  • Coupling is the measure of the strength of association established by a connection from one module to another. Strong coupling complicates a system since a module is harder to understand, change, or correct by itself if it is highly interrelated with other modules. Complexity can be reduced by designing systems with the weakest possible coupling between modules
  • Cohesion measures the degree of connectivity among the elements of a single module (and for object-oriented design, a single class or object).

In other words, coupling is a dependency measure among classes, and cohesion is how the attributes and methods of the same class are related.

What's more it's important to highlight the meaning of code smell. It first appears in Martin Fowler's book, Refactoring. There is a whole chapter, written with Kent Beck, that talks about code smells. So, who is better to explain it than Martin Fowler?

Code smell isn't necessary a bad code, it's an indicator that something could be wrong, but it depends so much on the context.

Last but not least, I want to talk about technical debt. What is it?

Short answer: it's a conscious decision to get a better understanding.

Long answer: Technical debt is a metaphor coined by Ward Cunningham which represents a bank debt: you borrow a loan to get money fast to do something ASAP, but you should pay interests. It happens the same with software since you can decide to have a technical debt to release something fast to learn and get a better understanding of the context, and then pay the debt off with refactoring later. So, it's not about writing bad code, you write the code the best you can with you current understanding, that is the debt. The important thing here is to pay the debt off because, like a loan, if you don't pay, you must pay more interest later.

Bad code, on the other hand, is not always a conscious decision, most of the time is due to a lack of skill.

With these concepts clarified, let's see some examples in my website code. Some of them are bad code, some others are technical debt, and the reason for that was this article and maybe some articles about the refactoring that I need to do to pay the debt off. ;-)

Code smell examples

Now I'm going to show you some code smells and explain them to you with the examples below.

First

Location: raiseexception/admin/views.py

@login_required
async def comments_view(request: Request):
    pending_comments = await PostComment.filter(
        state=PostCommentState.PENDING.value
    )
    comments = []
    # TODO: kinton - refactor this using prefetch from DB
    for pending_comment in pending_comments:
        await pending_comment.post.fetch()
        comments.append(pending_comment)
    if request.method == 'POST':
        data = await request.form()
        approved_comment_ids = data.getlist('approve')
        # TODO: kinton - refactor this implementing update method and in lookup
        mail_client = MailClient()
        mail_tasks = []
        for comment_id in approved_comment_ids:
            comment = await PostComment.get(id=int(comment_id))
            comment.state = PostCommentState.APPROVED
            await comment.save(update_fields=('state', 'modified'))
            if comment.email:
                await comment.post.fetch()
                post = comment.post
                mail_tasks.append(mail_client.send(
                    to=To(email=comment.email, name=comment.name),
                    subject='Comment approved',
                    message=f'Hi {comment.name}, you comment was approved. '
                            f'Check it <a href="{settings.SITE}/blog'
                            f'/{post.title_slug}">here</a>'
                ))
        await asyncio.gather(*mail_tasks)
        return RedirectResponse(url='/admin/blog/comments', status_code=302)

    return settings.TEMPLATE.TemplateResponse(
        name='/admin/comments.html',
        context={'request': request, 'comments': comments}
    )

Code smells:

  1. This function has low cohesion, that makes that the view get many responsibilities, such as:
    • Approve comments on posts.
    • Send email to commentators when a comment is approved.
    • Query for pending comments.
    • Input data validations. This is the only responsibility of the view should have. It's not in the code, by the way. XD
  2. Even when the TODOs are related to a project that I have as a hobbit, Kinton and those features aren't implemented yet, they could be encapsulated in PostComment model.
  3. The view is changing the PostComment state directly. It would be better if a controller send approve message to PostComment object. That's something like post_comment.approve().
  4. The function has 37 lines of code. That is a lot.
  5. The first part of the view is executed both request method is GET and POST but it's only relevant when the request method is GET

Second

Location: raiseexception/admin/views.py

@login_required
async def publish_post_view(request: Request):
    data = await request.form()
    post = await Post.get(id=int(data['post_id']))
    post.state = PostState.PUBLISHED
    post.published_at = datetime.datetime.now()
    await post.save()
    subscriptions = await Subscription.filter(verified=True)
    email_client = MailClient()
    emails_to_send = []
    for subscription in subscriptions:
        emails_to_send.append(email_client.send(
            to=To(email=subscription.email, name=subscription.name),
            subject='A new post was published!',
            message=f'Hi {subscription.name}, a new post was published...'
        ))
    await asyncio.gather(*emails_to_send)
    return RedirectResponse(
        url=f'/admin/blog/{post.title_slug}',
        status_code=302
    )

Code smells:

  1. Many responsibilities are caused by low cohesion. What about if I want to do more things when a post is published? Something like posting the article on Twitter and/or LinkedIn would add more responsibilities to the view.
  2. View is changing the PostState state directly. It's similar to the first example.

Third

Location: raiseexception/mailing/client.py

async def send(self, to: To, subject: str, message: str):
    async with httpx.AsyncClient(auth=(self._username, self._password)) \
            as client:
        response = await client.post(
            url=f'{self._base_url}/send',
            headers={'Content-Type': 'application/json'},
            json={
                'SandboxMode': self._sand_box,
                'Messages': [
                    {
                        'From': {
                            'Email': 'no-reply@raiseexception.dev',
                            'Name': 'RaiseException'
                        },
                        'To': [
                            {
                                'Email': to.email,
                                'Name': to.name
                            }
                        ],
                        'Subject': subject,
                        'HTMLPart': message
                    }
                ]
            }
        )
    return not response.is_error

Code smells:

  1. I'm using an external library directly. What about if I want to change httpx for another library in the future? Right now is just a simple change because it's only used in one place, but if I use that library in many places, I would need to change many places just for one reason.
  2. I'm not handling possible exceptions. What about if the mailing service is down? What about if I can't establish a connection for other reasons?

Fourth

Location: raiseexception/blog/models/post.py

class Post(Model):
    _id = fields.IntegerField()
    _title = fields.CharField()
    _title_slug = fields.CharField(immutable=True)
    _body = fields.CharField()
    _description = fields.CharField()
    _state = fields.CharField(choices=PostState, default_value=PostState.DRAFT)
    _category = fields.ForeignKeyField(to=Category)
    _author = fields.ForeignKeyField(to=User)
    _published_at = fields.DatetimeField()
    _created_at = fields.DatetimeField(auto_now_add=True)
    _modified_at = fields.DatetimeField(auto_now=True)

    class Meta:
        db_table = 'posts'

Code smells:

  1. Kinton use Active record approach, just like Django or RoR do. What's the problem with this approach? Model is coupled with details. How we store data is a detail. For example, Post model has a class Meta with a db_table attribute. Why a domain model need to know what is the name of the table in DB? A different approach could be repository approach. Both of them have pros and cons (like everything) and I'm learning about repository approach and I could apply that learning in Kinton, I think so ;-).

Fifth

Location: tests/integration_tests/admin/views/test_post.py

def test_get_posts(db_connection, event_loop, test_client, cookies_fixture):
    category1, category2, category3 = event_loop.run_until_complete(
        CategoryFactory.create_batch(3)
    )
    response = test_client.get('/admin/blog', cookies=cookies_fixture)

    assert response.status_code == 200
    assert '<form id="create-post" action="/admin/blog" method="post">' \
        in response.text
    assert '<label for="title">Title:</label>' in response.text
    assert '<input id="title" name="title" type="text" required autofocus>' \
        in response.text
    assert '<label for="category_id">Category:</label>' in response.text
    assert '<input id="category_id" name="category_id" list="category_values' \
           '" required>' in response.text
    assert '<datalist id="category_values">' in response.text
    assert f'<option value="{category1.id}">{category1.name}</option>' \
        in response.text
    assert f'<option value="{category2.id}">{category2.name}</option>' \
        in response.text
    assert f'<option value="{category3.id}">{category3.name}</option>' \
        in response.text
    assert '</datalist>' in response.text
    assert '<label for="description">Description:</label>' in response.text
    assert '<input id="description" name="description" type="text" required>' \
        in response.text
    assert '<label for="body">Body:</label>' in response.text
    assert '<textarea id="body" name="body" required></textarea>' \
        in response.text
    assert '<input type="submit" value="Create">' in response.text
    assert '<p>post created successfully</p>' not in response.text

Code smells:

  1. I'm doing assertions with strings using in. This makes the test so fragile because I can't add/change tag attributes without break the test. It's my first time writing tests with HTML. So, I did that to get a better understanding. Maybe I could replace it with a tool to get data of HTML like beautifilsoup, but I'm not sure yet.

Wrapping up

These are some examples of bad code/code smells that I have in raiseexception code. I have many more, but they are based on, mostly, the same reasons. Probably, I'll write some posts to pay the technical debt and improve the bad code to have a better design. This website is my lab. :D

One important thing is to stop calling technical debt to bad code and be conscious it's a good thing to have technical debt. I share with you all this short video of Ward Cunningham talking about it.

See you next time and any feedback is welcome!

Leave a comment

    Comments:

  • Duvanar,

    The code smell is interesting, i think it happened to me, it's like a sixth sense. could this technical debt be paid at the end of each user requirement or the end of the program?

  • Julián,

    @duvanar the idea is to pay the debt as soon as possible and you can be done bit by bit while you're getting more understanding.