Code smells for model-view-controller architectures

Code smells are poor design and implementation choices that hinder comprehensibility and maintainability of code. Aniche, Bavota, Treude, Gerosa, and Van Deursen introduce a catalog of six code smells in Web applications that make use of the model-view-controller pattern.

Many studies have shown that code smells make code less maintainable and more prone to bugs. Some smells cause code to be changed more often due to violations of the single responsibility principle, which states that a class or module should have only one reason to change.

Most of these studies are based on a catalog of code smells that was originally defined by Martin Fowler and Kent Beck in Refactoring. The smells are generally applicable to any system written in an object-oriented manner, but overlook the role that a class or module may have in the system’s architecture.

This is why we have to study smells for specific architectures, like MVC. We must learn about their characteristics and impact, so that developers can understand how to avoid those smells and static analysis tools know how to recognise them.

The study makes use of surveys, interviews, and repository mining.

The authors started with a three-step data gathering process:

1. A simple survey that asked respondents to list good and bad practices for dealing with models, views, and controllers. The survey yielded 22 complete responses.

2. A more comprehensive survey that aimed to elicit good and bad practices for each of the five major MVC roles (controller, entity, service, component, and repository). This survey was completed by 14 respondents.

3. Unstructured interviews were held about good and bad practices for each of the five roles. The authors interviewed 17 professional developers, all of whom were familiar with the Spring MVC framework.

Two authors performed an open coding process to group good and bad practices into categories. Practices that were not specific to the MVC pattern were discarded.

The coding process resulted in a list of nine possible smells, which were presented to a core Spring MVC maintainer. Three of the smells were removed because they were specific to Spring and therefore not likely to affect users of other MVC frameworks.

To understand the characteristics and impact of code smells, the authors:

• analysed 120 Spring MVC projects that are hosted on GitHub;

• asked 21 Spring MVC developers to take part in a survey that assessed their perception of the six code smells;

• looked for experts in development of MVC applications using frameworks other than Spring.

I’ll list the code smells first, and discuss their characteristics and impact afterwards.

Six MVC smells were identified.

Promiscuous controller

A controller should provide cohesive operations and endpoints to clients, i.e. it should depend on a limited number of services (at most 3) and handle at most 10 routes (side note: The values 3 and 10 were derived using a formula that can be found in the original article. Don’t think of these (and other numerical values in this section) as absolute thresholds; they’re more like rules of thumb.).

Promiscuous controllers can be broken up into two or more classes until each controller is no longer promiscuous.

Brain controller

Flow control in controllers should be very simple, e.g. ideally a controller shouldn’t interpret input to determine what actions to take. Since a controller with a lot of flow control will be littered with method invocations, the authors argue that the number of non-framework methods that can be executed by a controller should never exceed 55.

Since business logic is supposed to be implemented in model layer, it has no place in controllers. Any business logic in a brain controller should be moved to an entity, component, or service class.

Meddling service

A service should contain business logic and/or handle business logic among domain classes, but never contain SQL queries.

Data access must always be handled by repositories instead.

Brain repository

Repositories are meant to handle anything related to data persistence, but should not contain complicated business logic or complex queries (side note: More specifically, queries that join multiple tables with complex filters, queries that are constructed dynamically, or objects that are manually assembled from query results.).

One could therefore argue that a brain repository is a class whose McCabe’s complexity exceeds 24 or SQL complexity exceeds 29. Complex logic and SQL queries should live in different methods. Logic that’s used by multiple repositories should be in a component.

Laborious repository method

Methods should have only one responsiblity and do one thing. For a repository method, this means that it should execute only one query.

Methods that execute multiple queries should be split, so that each method only executes one query. Methods can be private or public, depending on whether the persistence action makes sense on its own.

Fat repository

Each repository should only deal with a single entity, otherwise it loses its cohesion and becomes harder to maintain.

If a repository deals with multiple entities, each entity should get its own dedicated repository.

The most common smell in the analysed dataset is the fat repository (20.5%), followed by the promiscuous controller (12.2%) and the brain controller (7.4%).

Brain controllers and laborious repository methods are often also affected by the traditional “complex class” code smell and in 59% of cases a brain controller is also a “God class”. The other smells do not appear to overlap at all with traditional code smells, which suggests that the smells in this catalog really are from a distinct category.

Impact on change- and defect-proneness

Analysis of Spring MVC projects shows that classes affected by MVC and traditional smells are significantly more prone to changes (almost 3 times more likely) and defects (2 times more likely).

These differences become smaller when artifact size is taken into consideration (side note: More code means more opportunities for bugs to appear, so this isn’t very surprising of course): classes affected by smells are still prone to changes, but are not more defect-prone.

There are also differences between MVC smells and traditional smells: the latter have a stronger negative impact on change- and defect-proneness.

Of the six MVC smells, the brain repository and meddling service have the strongest impact on change-proneness, while the meddling service is the only MVC smell that clearly results in more bug-fixing activities.

Perception by developers

Developers clearly perceive classes affected by MVC smells as problematic, particularly in the case of the meddling service, fat repository, and brain controller smells.

Some developers were able to correctly identify and define the smells without prior knowledge of the authors’ catalog. On the other hand, over half of all participants did not perceive classes affected by the laborious repository method as problematic.

Introduction and survival

Once an MVC smell is introduced in a system, it tends to survive for quite a long time. In general, there’s more than 50% chance that a smell will survive for longer than 500 days. Fat repositories even have an 80% chance of surviving more than 1,500 days. 69% of smells are never removed at all.

These smells are not always caused by code aging (side note: Do keep in mind that this analysis was done on open source projects. Closed-source projects may have different characteristics.): some smells already exist when the code artifact is first committed to the repository. For laborious repository methods this even happens 86.5% of the time!

Generalisability to other frameworks

Most of the identified smells are generalisable to other frameworks (specifically VRaptor, Ruby on Rails, ASP.NET MVC, and Play!).

Application built using frameworks that use the active record pattern don’t appear to suffer from meddling services and generally don’t use repositories, which would essentially eliminate three of the MVC smells. Instead, they’re more likely to suffer from fat models.