Forem: Manuel Rivero

A case of Shotgun Surgery

Manuel Rivero — Tue, 18 Mar 2025 17:39:42 +0000

The context.

Some time ago we were collaborating with a client whose product relied heavily on Search engine optimization (SEO). In general, SEO is usually an important area to focus on, but for this client, SEO represented a significant source of income and was therefore critical to their success.

When search engines analyze a page, there are two key aspects our system needs to determine, the indexing and the canonical of that page:

Indexing: it determines whether a page should be indexed or not by the search engine.
Canonicalization: it specifies the canonical URL of similar pages to avoid penalties for duplicate content.

For that product, SEO was not only a significant source of revenue but also quite complex. Decisions about indexing and canonical URLs depended on factors such as location, search types, and the number of results. This complexity was inherent to the domain problem, i. e., its essential complexity.

The code

In that legacy system, SEO rules were located in two areas of the application: the IndexingCalculator and the CanonicalCalculator classes.

Clients viewed these two functionalities through the following interfaces shown below:

public interface IndexationCalculator {
  Page update(Page current, Search search);
}

public interface CanonicalCalculator {
  Page update(Page current, Search search);
}

public class Page {
  public bool isIndexed;
  public String canonical;
  public String url;
}

The problem.

At first glance, based on the names of the interfaces, and, it seemed that the responsibilities were neatly separated. There was one place for deciding whether a page should be indexed, IndexingCalculator, and another for calculating its canonical URL, CanonicalCalculator. Both came with their own suite of tests that verified their respective behavior independently.

However, when asked to modify the canonical calculation rules, we were forced to consider and likely modify both the implementation of IndexingCalculator and CanonicalCalculator. The same happened when we had to modify the indexing rules. The problem was that changes to one component often required changes to the other.

Although the interfaces were designed to separate responsibilities and their names aligned with domain concepts, their implementation was not cohesive. Some indexing logic had leaked into CanonicalCalculator, and some canonical logic had leaked into IndexingCalculator^[1]. The code exhibited a clear case of a code smell known as Shotgun Surgery which is a violation of the Single Responsibility Principle (SRP). This was a source of accidental complexity.

In this specific case, there was a lack of cohesion at the implementation level. Although the axes of change had been correctly identified at the interface level, the responsibilities were scattered across both implementations.

A case of shotgun surgery...

To fix this case of Shotgun Surgery we needed to segregate responsibilities by moving all the indexing logic to IndexingCalculator, and all the canonical logic to CanonicalCalculator so that we no longer violate the SRP.

Segregating the Rules.

If the canonical and indexing logics could be executed in any order, moving the rules would simply involve moving test cases between the tests of each implementation and moving the corresponding code using Move Function.

Moving rules would be easy if the order of execution of the canonical logic and the indexing logic did not matter.

However, in this case, fixing the shotgun surgery was much more difficult because the order in which the SEO rules had to run was neither in IndexingCalculator nor in CanonicalCalculator but in client code that wasn’t even tested. This meant that there was no way to determine whether moving some logic from one class to the other was preserving the behavior.

Considering, as already explained, that SEO represented a significant source of revenue for the product, we could not accept the level of risk involved in refactoring without tests that protected all SEO behaviour.

To reduce the risk of altering the behavior, our strategy was to introduce a new class called PageSEO, whose responsibility was to coordinate the indexing and canonical calculations. PageSEO explicitly owned the sequence of operations. By centralizing this orchestration logic, we eliminated the implicit, fragile dependencies embedded in the client code.

public class PageSEO {
  Page calculate(Page current, Search search) {
    // ...
  }
}

Next, we created a new suite of tests for PageSEO. This new suite not only included the test cases of both CanonicalCalculator and IndexingCalculator, but also new previously missing test cases that protected the order in which the indexing and canonical rules were applied. In this way, we were testing not only each component individually but also the integration between them.

New tests for the new abstraction that encapsulates the order in which indexing and canonical logics are executed.

These tests for PageSEO gave us enough confidence to start refactoring.

One rule at a time, we started moving responsibilities to their rightful owners, the indexing logic that had leaked into CanonicalCalculator back to IndexingCalculator, and the canonical logic that had leaked into IndexingCalculator back to CanonicalCalculator. We deleted tests that were testing logic that was located in the wrong places and used the Move Function refactoring on the logic.

Once we finished segregating responsibilities, we removed the redundant tests that were only addressing each separate responsibility (the tests directly against CanonicalCalculator and IndexingCalculator).

Now we can move the rules with confidence. Once the responsibilities are segregated, we delete the initial tests.

After applying this refactoring, we achieved the following:

Cohesive Objects: CanonicalCalculator and IndexingCalculator handled only their own responsibilities.
Centralized Coordination: The order of execution was neatly encapsulated within PageSEO. The calculations remained independent, but their interaction was controlled and tested.

The SEO code adhered to the SRP.

Conclusion.

We have presented a real case of Shotgun Surgery and explained how we refactored it to achieve a more cohesive design.

In simpler situations, addressing Shotgun Surgery might only involve moving logic and tests. But in this context, the untested execution order of the rules, domain complexity, and the high business value of SEO forced us to take a more careful approach. By introducing an intermediate coordinator, PageSEO, we reduced risk and provided a single place to test and reason about SEO's behavior.

If you find yourself in a similar situation, perhaps you can apply some aspects of this solution to reduce risks.

I hope this explanation was clear and helpful. Feel free to reach out for guidance if you encounter similar challenges or need assistance in improving the design of your project!

Notes.

[1] The reasons why the responsibilities became mixed might lie in the design of the interfaces, but this issue may be addressed in a future post.

Relevant mutants

Manuel Rivero — Tue, 11 Mar 2025 17:47:08 +0000

1. Introduction.

Mutation testing gives us information about the fault detection capability of a test suite, whether the assertions that its tests make are good and strong enough to capture regressions.

The idea behind it is to purposefully inject a regression in a copy of the code, “the mutant”, run the test suite against this mutant and check if the test suite breaks:

If it does, everything is fine, “the mutant is killed”. This means that the test suite protects us from that kind of regression.
If it does not, “the mutant survives”. This means that the tests don’t protect us from that regression happening. We may have found something to improve in the tests.

How mutation testing is used.

However, when we apply mutation testing to legacy code, not all surviving mutants are a signal of weaknesses in the test suite. Some of the survivors may be related to the existence of dead code, and others may be indicating code that is unnecessary to produce the expected behaviour, and, as such, could be simplified. That is why we must analyse each of the surviving mutants to decide if it is relevant or not, and only if it is so, write new test cases to kill it.

Let's see it with an example.

2. Analyzing the surviving mutants in the Crazy Portfolio kata.

The Crazy Portfolio kata is a kata to practise working with legacy code that we recently published. This kata is complicated because the code of the Portfolio class, not only has many responsibilities and complex conditional logic, but also, uses dates and time zones, and has side-effects, many boundary conditions between partitions and some implicit invariants^[1].

In the code used for this example we had already written characterization tests achieving the maximum possible branch coverage, which is not 100% because there is an unreachable branch in the code, and because of the seams we introduced using Extract and Override Call, which, of course, are not executed in the tests.

Depending on the refactoring we plan to do, it might not be enough to have high branch coverage to start refactoring with confidence, so we applied mutation testing using Stryker and the results were that 36 out of 160 mutants survived.

Apparently, we would need to kill a lot of mutants by adding new test cases in order to improve our test suite, and perhaps some of the mutants might not be so easy to kill.

If we analyse the surviving mutants, we'll see that there really aren't that many mutants to kill.

The surviving mutants fall into the following categories:

Mutants in dead code.
Mutants in seams.
Mutants in code not used in the tests because of the seams.
Mutants in superfluous code that don’t point to weaknesses in the test suite.
Mutants that do point to weaknesses in the test suite.

2. 1. Mutants in dead code.

This category includes all the mutations in the non-accessible branch in line 60 that we identified when we wrote the characterization tests.

Surviving mutations in dead code are irrelevant.

These surviving mutants are obviously irrelevant.

2. 2. Mutants in seams.

All the mutants in the seams that we introduced, as for instance, the createDate method seen in the following fragment of the Stryker report, are not relevant because the seams’ code is not executed in the tests.

Surviving mutations in seams are irrelevant.

The same happens for the mutants inside the other seams, the protected methods: formatDate, getAssetLines, displayMessage and getNow.

2. 3. Mutants in code not used in the tests because of the seams.

In this group we find the mutation that survives in the constructor of the Portfolio class,

Surviving mutations in unused code due to seams are irrelevant.

Since we introduced a seam to get the assets without actually reading the file in the unit tests, the portfolioCsvPath field is not used. Therefore this mutant survival is related to the getAssetLines seam that we introduced, and this means that it is not a relevant mutant.

About the categories we have seen so far.

The surviving mutants in the three categories we’ve seen so far can be quickly and safely ignored because they don’t provide any hints on how to improve the test suite or the production code.

We’ll need to examine one by one the rest of the surviving mutants to see if they actually point to weaknesses in our tests or production code that may be simplified. Let’s see the two remaining categories.

2. 4. Mutants in superfluous code that don’t point to weaknesses in the test suite.

Let's look at the most interesting examples:

Case 1: Mutations surviving in line 29.

Have a look at:

Surviving mutation changing / operator to * operator in line 29.

or at:

Another surviving mutation changing / operator to * operator in line 29.

There are several more similar mutations on the same line.

These mutations that replace the division operator with the product operator seem very confusing and may, at first, be baffling. Why do they survive?

To understand it, we need to look closer at the condition and what it means. The only relevant thing to make the condition true or false is the sign of the difference of the two times in milliseconds: asset.getDate().getTime() - now.getTime(). The rest of the expression, (1000 * 60 * 60 * 24), is just a positive number that is dividing the previously mentioned difference, but it will not change its sign, and therefore, it will not change the evaluation of the boolean expression Math.floor(difference / positive_number) < 0 .

These mutants, that change / operators to * operators, therefore, are not pointing to weaknesses in our tests. What they are indicating, instead, is production code that we don’t need, and, which we could simplify^[2], writing the following condition instead: Math.floor(asset.getDate().getTime() - now.getTime()) < 0.

This simplification (that we’d do only once we have stronger tests that eliminate all the relevant mutants) would be enough to eliminate all the related mutants.

The only surviving mutant in line 29 that is relevant to improve our tests is the one that changes the < operator to the <= operator.

Surviving mutation changing < operator to <= operator in line 29.

This surviving mutant indicates that our tests are not correctly exercising a boundary condition that exists between two partitions of the values that the difference between the date of the assets and the date the portfolio value is being calculated can take.

Case 2. Mutations surviving in line 27.

Surviving mutation when mapping to value object in line 27.

The survival of those mutations means that the PricelessValue class is not necessary to achieve the behaviour that the tests are protecting. How do they survive?

These mutations survive because the derived PricelessValue class is not used at all in the code. The only thing that is used is the getter of the base object of the hierarchy, MeasurableValue, and therefore the entire inheritance hierarchy is superfluous. The root cause of this problem is the feature envy against the Asset class present in the current code.

Again these mutants are not relevant to improve our tests and we do not have to waste time trying to kill them.

Once we have stronger tests that eliminate all the relevant mutants, we can refactor the code to eliminate the rampant feature envy, and decide if we need to keep the hierarchy or not. These mutants would disappear then.

5. Mutants that do point to weaknesses in the test suite.

We’ve already seen an example in line 29 of a mutant that was indicating that our tests are not correctly exercising a boundary condition. This surviving mutant is changing the < operator to <=.

Surviving mutation changing < operator to <= operator in line 29.

There are other surviving mutants similar to the previous one that also indicate problems testing boundary values, for instance the ones in the lines 32 and 46 presented below:

Surviving mutation in line 32 related to problems testing boundary values.

Surviving mutation in line 46 related to problems testing boundary values.

Another example of a surviving mutant that may point to a weakness in our test suite is the one in line 22 showed below:

Surviving mutation in line 22 related to not checking an exception message.

If we examine the test suite, we’ll see that we are testing that an error is thrown when the date is badly formatted, but we are not checking the error message. In this case, depending on if we want the tests to be coupled with the ”wrong date” string or not, we may decide to kill this mutant or not. We decided to ignore this mutant to avoid coupling the tests to the exception message.

Conclusions.

We’ve shown an example of how not all surviving mutants point to weaknesses in our test suites. Of the original 36 surviving mutants, only 13 of them were relevant to improve our tests suite^[3] (we chose to ignore the one related to the error message). Those 13 mutants are related to boundary conditions that are not being properly exercised by the test suite.

Having to kill 13 mutants instead of 36 is a much less daunting task than killing 36 of them. A nice thing is that most of the discarded, not relevant mutants were easy to identify: the ones related to the existence of dead code^[4] and to the introduction of seams. This is great because, sometimes, surviving mutants might not be so easy to kill^[5].

We also saw another category of surviving mutants that, even though, do not point to improvements in the test suite, are a signal of code that may be unnecessary to achieve the desired behaviour. We may decide to take advantage of these hints in later refactorings once the test suite has improved by killing the relevant mutants.

I hope this detailed example may prove useful to you when you analyse mutation testing results.

Acknowledgements.

I’d like to thank Fran Reyes and Rubén Díaz for revising drafts of this post.

Also thanks to Audiense’s developers for being the beta testers of the Crazy Portfolio kata as part of our deliberated practice sessions. It is always a pleasure to work with you.

Finally, I’d also like to thank Carlos Machado for the photo.

Notes.

[1] We use a simplified version of this kata in our Changing Legacy Code training.

[2] We already talked about surviving mutants pointing to redundant code in a previous post: Mutando para simplificar.

[3] The 13 relevant surviving mutations are in lines 29 (1), 32 (2), 46 (2), 70 (1), 71 (2), 76 (1), 77 (2) and 83 (2).

The surviving mutations in lines 32, 46, 77, and 83 point to boundary conditions which are completely ignored by the suitcase (that's why one of the surviving mutations can completely remove the condition), whereas the surviving mutations in lines 29, 70 and 76 point to partially tested boundary conditions.

[4] We advise using a coverage tool to detect uncovered and dead code before running a mutation testing tool, because coverage tools results are easier to analyze than the mutation testing tools' ones.

[5] Testing boundary conditions, and, hence, killing mutants related to them becomes much easier if you know about boundaries' on and off points. These knowledge provides you a systematic way if killing this kind of mutants. We teach these concepts in our revamped TDD course material.

On code smells catalogues and taxonomies

Manuel Rivero — Fri, 07 Mar 2025 12:06:36 +0000

This post is an English translation of the original post: De taxonomías y catálogos de code smells.

Index.

Introduction: code smells and refactoring.
Martin Fowler: code smells and catalogues.
Taxonomies.

Wake’s taxonomy 2003.
Mäntylä et al taxonomy 2003.
Mäntylä et al taxonomy 2006.
Jerzyk et al taxonomy 2022.

Jerzyk’s extended code smells catalogue.
Online catalogue.

Conclusions.
Acknowledgements.
References.

Introduction: code smells and refactoring.

Refactoring is a practice that allows us to sustainably evolve code. To refactor problematic code, we first need to be able to recognize it.

Code smells are descriptions of signals or symptoms that warn us of possible design problems. It’s crucial to detect these problems and remove them as soon as possible.

Refactoring is cheaper and produces better results when done regularly. The longer a problematic piece of code lives, the more it takes to refactor it, and the biggest friction it will add to evolve new features directly contributing to accruing technical debt. Sustaining this situation over a long period of time may have a very negative economic impact. In the worst case scenario, the code might get too complicated to keep on maintaining it.

Cost of introducing a feature over time (image from The Three Values of Software from J. B. Rainsberger).

Another, sometimes not so visible, consequence of bad software quality is its effects on developers. Less refactoring, can lead to a less maintainable code that can lead to longer times to develop new features, which, in turn, means more time pressure to deliver, which can lead us to test less, which will allow us to refactor less… This vicious circle might have a very demoralising effect.

Less refactoring vicious circle.

Understanding and being able to identify code smells gives us the opportunity of detecting design problems when they are still small and located in specific areas of our code, and, as such, are still easy to fix. This might have a very positive economic and emotional effect.

The problem is that code smells are not usually very well understood. This problem is understandable because code smells descriptions are sometimes abstract, diffuse and open to interpretation. Some code smells seem obvious, others not so much, and some might mask other smells.

Besides, we should remember that smells are only symptoms of possible design problems, not guarantees of the existence of problems. To further complicate matters, in addition to possible false positives, there are degrees to the problem that each smell represents, trade-offs between different smells and contraindications in their refactorings (sometimes "the cure is worse than the disease").

Therefore, recognizing design problems through code smells is a subtle skill that requires experience and judgement. Acquiring this skill, which sometimes may feel like a kind of spider-sense, can take some time.

In our experience teaching and coaching teams, not being able to identify code smells is one of the greatest barriers to refactoring. Many developers do not detect design problems while they are still small and localised. What we often observe is that they don't sense the design problems until the problems are quite large and/or have compounded with other problems, spreading through their codebase.

Code smells catalogues.

In 1999 Fowler and Beck published a code smells catalogue in a chapter of the book,
Refactoring: Improving the Design of Existing Code. This catalogue contains the descriptions of 22 code smells.

In 2018 Fowler published a second edition of his book. In this new edition there are a number of changes with respect to the first, mainly in the code smells catalogue of smells and the refactorings catalogue (detailed in Changes for the 2nd Edition of Refactoring).

If we focus on the smells, which is the goal of this post, the changes are as follows:

Four new code smells are introduced: Mysterious Name, Global Data, Mutable Data and Loops.
Two code smells are removed: Parallel Inheritance Hierarchies and Incomplete Library Class.
And four code smells are renamed: Lazy Class becomes Lazy Element, Long Method becomes Long Function, Inappropriate Intimacy becomes Insider Trading, and Switch Statement becomes Repeated Switches.

There are 24 code smells in total. Even though they usually have catchy and memorable names, it’s difficult to remember such a long list.

How could we better understand code smells? How could we remember them more easily?

In this post, and some later ones, we will talk about strategies to better understand and remember code smells.

Organisational strategies.

Organising and manipulating information, seeing it from different points of view, can help us understand and remember it better. Taxonomies are a type of organisational strategy that allows us to group study material according to its meaning. This helps to create significant groupings of information or "chunks" that will facilitate learning.

As we mentioned, the 2018 Fowler catalogue is a flat list that does not provide any type of classification. Although, reading the descriptions of the code smells, and the motivation sections of the different refactorings, we can glimpse that some smells are more related to each other than to other smells, these relationships are not expressed explicitly and remain blurred and dispersed in different parts of the book.

The use of taxonomies to classify similar code smells can be beneficial to better understand and remember code smells, and recognize the relationships that exist between them.

Taxonomies.

There have been different attempts to classify code smells according to different criteria. The most popular classification is that of Mäntylä et al 2006 but it is not the first one. Below we will show some taxonomies that we consider quite interesting.

Wake 2003.

Wake in his book, Refactoring Workbook, describes 9 new code smells that did not appear in Fowler's original catalogue: Dead Code, Null Check, Special Case, Magic Number, Combinatorial Explosion, Complicated Boolean Expression, and three related to bad names: Type Embedded in Name, Uncommunicative Names and Inconsistent Names.

Wake explicitly classifies code smells by first dividing them into two broad categories, Smells within Classes and Smells Between Classes, depending, respectively, on whether the code smell can be observed from within a class or whether a broader context needs to be considered (various classes). Each of these categories is divided into subcategories that group code smells based on where they can be detected. This classification criterion, later called “occurrence" by Jerzyk, answers the question: "where does this code smell appear?".

Following this “occurrence" criterion Wake finds 10 subcategories.

Within the category of Smells within Classes we find the following subcategories:

Measured Smells: code smells that can be easily identified with simple length metrics.
Names: code smells that create semantic confusion and affect our ability to create mental models that help us understand, remember, and reason about code.
Unnecessary Complexity: code smells related to unnecessary code that adds mental load and complexity. Dead code, YAGNI violations, and accidental complexity.
Duplication: developer's nemesis. These code smells generate more code to maintain (cognitive and physical load), increase error proneness, and make it difficult to understand the code.
Conditional Logic Smells: code smells that complicate conditional logic making it difficult to reason about and change, and increasing error proneness. Some of them are weak substitutes for object-oriented mechanisms.

The subcategories within the Smells between Classes category are:

Data: code smells in which we find either pseudo objects (data structures without behaviour), or we find that some missing abstraction is missing.
Inheritance: code smells related to misuse of inheritance.
Responsibility: code smells related to a bad assignment of responsibilities.
Accommodating Change: code smells that manifest when we encounter a lot of friction when introducing changes. They are usually caused by combinations of other code smells.
Library Classes: code smells related to the use of third-party libraries.

Wake’s taxonomy 2003 (new code smells appear in green).

Wake presents each smell in a standard format with the following sections: Smell (the name and aliases), Symptoms (clues that may help detect it), Causes (notes on how it may have generated), What To Do (possible refactorings), Benefits (how the code will improve by removing it) and Contraindications (false positives and trade-offs). In some cases, he adds notes relating the code smell to design principles that might have helped to avoid it.

The book also contains many practical exercises and very useful tables (symptoms vs. code smells, smells vs. refactorings, reverse refactorings, etc.) and exercises that relate code smells to other concepts such as design principles or design patterns.

We highly recommend reading this book to delve into the discipline of refactoring and better understand when and why to apply the refactorings that appear in Fowler’s catalogue.

Although we also refer to to the rest of the taxonomies that we talk about in this post, the content about code smells in our course Code Smells & Refactoring is mainly based on Wake’s taxonomy seasoned with a bit of our own experience.

Mäntylä et al 2003.

In this taxonomy, code smells are grouped according to the effect they have on code (the type of problem, what they make difficult or the practices or principles that they break). This classification criteria is called "obstruction" by Jerzyk 2022.

The original classification from 2003 (A Taxonomy and an Initial Empirical Study of Bad Smells in Code) is comprised of 7 code smells categories: Bloaters, Object -Orientation Abusers, Change Preventers, Dispensables, Encapsulators, Couplers and Others.

Mäntylä et al taxonomy 2003.

This is how they define each of the categories:

Bloaters: this category of smells represents something in the code that has grown so large that it can no longer be handled effectively.
Object-Orientation Abusers: this category of smells relates to cases where the solution does not fully exploit the potential of object-oriented design.
Change Preventers: this category refers to code structures that make it very difficult to change the code.
Dispensable: these smells represent something unnecessary that should be removed from the code.
Encapsulators: this category has to do with communication mechanisms or data encapsulation.
Couplers: these smells represent cases of tight coupling, which goes against object-oriented design principles
Others: this category contains the remaining code smells (Comments and
Incomplete Library Class) that did not fit into any of the previous categories.

In their paper, Mäntylä et al discuss the reasons why each code smell is in a given category and not another, although they admit that some of them could be classified in more than one category.

Mäntylä et al 2006.

In 2006 Mäntylä et al published another paper (Subjective evaluation of software evolvability using code smells: An empirical study) in which they revised their original classification from 2003.

In this new version, they eliminated the Encapsulators (moving the Message Chains and Middle Man smells to the Couplers category) and Others (Comments and Incomplete Library Class disappear from the taxonomy) categories, and moved the Parallel Inheritance Hierarchies code smell from the Object-Orientation Abusers category to the Change Preventers category.

This version of their taxonomy is the one that has become most popular on the internet (it can be found on many websites, courses and posts), probably due to the greater accessibility (readability) of the summary of the paper that appears on the web: A Taxonomy for "Bad Code Smells".

Mäntylä et al taxonomy 2006.

What is interesting is not so much the discussion of which category each smell should fall into, but rather to start thinking that a given smell can have different types of effects on the code and the relationships between these effects. In fact, subsequent classifications from the point of view of the effect of a smell on the code ("obstruction") consider it more useful not to lose the information that a code smell can produce several effects. This leads to no longer considering categories as exclusive, which makes it possible that the same smell can fall into several categories.

Jerzyk et al 2022.

In 2022 Marcel Jerzyk published his master's thesis, Code Smells: A Comprehensive Online
Catalog and Taxonomy and a paper with the same title. His research on code smells had three goals:

Providing a public catalogue that could be useful as a unified knowledge base for both researchers and developers.
Identifying all the possible concepts that are being characterised as code smells and determining possible controversies about them.
Assigning appropriate properties to code smells in order to characterise them.

To achieve these objectives, they carried out a review of the existing literature on code smells up to that moment, with special emphasis on code smell taxonomies.

In the thesis 56 code smells are identified and described, of which 16 are new original proposals (remember that Wake described 31 code smells and Fowler 24 in his last review).

Descriptions and discussions of each of these 56 code smells can be found in Jerzyk's master's thesis.

Analysing the classification criteria of the taxonomies proposed up to that moment, Jerzyk finds three significant criteria to categorise code smells:

Obstruction: this is the criterion used by Mäntylä et al to classify smells in their taxonomy, it’s also the most popular criterion. It informs us about the type of problem that a code smell causes in the code (what they make difficult or the practices or principles that they break). In the thesis they update the Mäntylä taxonomy, adding three new groups: Data Dealers, Functional Abusers and Lexical Abusers. Below we present a mental map that shows the classification of the 56 code smells according to only this criterion.

Jerzyk's taxonomy using only the obstruction criterion.

Expanse: inspired by Wake’s taxonomy, this criterion talks about whether the code smell can be observed in a reduced context (within a class) or if a broader context (between several classes) needs to be considered. Possible categories are Within Class and Between Classes.
Occurrence: Also inspired by Wake’s taxonomy, this criterion is related to the location where (or the method by which) a code smell can be detected. Possible categories are Names, Conditional Logic, Message Calls, Unnecessary Complexity, Responsibility, Interfaces, Data, Duplication and Measured Smells.

The following table shows the 56 code smells in Jerzyk’s thesis classified according to the three criteria discussed above:

Code Smell	Obstruction	Expanse	Occurrence
Long Method	Bloaters	Within	Measured Smells
Large Class	Bloaters	Within	Measured Smells
Long Parameter List	Bloaters	Within	Measured Smells
Primitive Obsession	Bloaters	Between	Data
Data Clumps	Bloaters	Between	Data
Null Check	Bloaters	Between	Conditional Logic
Oddball Solution	Bloaters	Between	Duplication
Required Setup/Teardown	Bloaters	Between	Responsibility
Combinatorial Explosion	Bloaters	Within	Responsibility
Parallel Inheritance Hierarchies	Change Preventers	Between	Responsibility
Divergent Change	Change Preventers	Between	Responsibility
Shotgun Surgery	Change Preventers	Between	Responsibility
Flag Argument	Change Preventers	Within	Conditional Logic
Callback Hell	Change Preventers	Within	Conditional Logic
Dubious Abstraction	Change Preventers	Within	Responsibility
Special Case	Change Preventers	Within	Conditional Logic
Feature Envy	Couplers	Between	Responsibility
Type Embedded In Name	Couplers	Within	Names
Indecent Exposure	Couplers	Within	Data
Fate over Action	Couplers	Between	Responsibility
Afraid to Fail	Couplers	Within	Responsibility
Binary Operator in Name	Couplers	Within	Names
Tramp Data	Data Dealers	Between	Data
Hidden Dependencies	Data Dealers	Between	Data
Global Data	Data Dealers	Between	Data
Message Chain	Data Dealers	Between	Message Calls
Middle Man	Data Dealers	Between	Message Calls
Insider Trading	Data Dealers	Between	Responsibility
Lazy Element	Dispensables	Between	Unnecessary Complexity
Speculative Generality	Dispensables	Within	Unnecessary Complexity
Dead Code	Dispensables	Within	Unnecessary Complexity
Duplicate Code	Dispensables	Within	Duplication
"What" Comments	Dispensables	Within	Unnecessary Complexity
Mutable Data	Functional Abusers	Between	Data
Imperative Loops	Functional Abusers	Within	Unnecessary Complexity
Side Effects	Functional Abusers	Within	Responsibility
Uncommunicative Name	Lexical Abusers	Within	Names
Magic Number	Lexical Abusers	Within	Names
Inconsistent Names	Lexical Abusers	Within	Names
Boolean Blindness	Lexical Abusers	Within	Names
Fallacious Comment	Lexical Abusers	Within	Names
Fallacious Method Name	Lexical Abusers	Within	Names
Complicated Boolean Expressions	Obfuscators	Within	Conditional Logic
Obscured Intent	Obfuscators	Between	Unnecessary Complexity
Vertical Separation	Obfuscators	Within	Measured Smells
Complicated Regex Expression	Obfuscators	Within	Names
Inconsistent Style	Obfuscators	Between	Unnecessary Complexity
Status Variable	Obfuscators	Within	Unnecessary Complexity
Clever Code	Obfuscators	Within	Unnecessary Complexity
Temporary Fields	O-O Abusers	Within	Data
Conditional Complexity	O-O Abusers	Within	Conditional Logic
Refused Bequest	O-O Abusers	Between	Interfaces
Alternative Classes with Different Interfaces	O-O Abusers	Between	Duplication
Inappropriate Static	O-O Abusers	Between	Interfaces
Base Class Depends on Subclass	O-O Abusers	Between	Interfaces
Incomplete Library Class	Other	Between	Interfaces

Some of the names in the table are different from those that usually appear in the literature. The changes in naming were due to the introduction of more updated names, as is the case for, for instance, Lazy Element or Insider Trading which were previously called Lazy Class and Inappropriate Intimacy, respectively.

Several smells are new:

Some, like Afraid to Fail, Binary Operator in Name, Clever Code, Inconsistent Style, and Status Variable, are completely new ideas.
Others are concepts that already existed in the literature but had not been considered in the context of code smells before: Boolean Blindness or Callback Hell.
Three of them propose alternatives for code smells that are being questioned in the literature: "What" Comment as an alternative to Comments, Fate over Action as an alternative to Data Class, and Imperative Loops as an alternative to Loops (see Jerzyk's thesis for more on why these original code smells are debatable).
Others generalise problematic concepts that have arisen in the literature: Complicated Regex Expression, Dubious Abstraction, Fallacious Comment, Fallacious Method Name.
Finally, a known problem (especially in the field of functional programming), which had not been considered as a code smell until now: Side Effects.

One super useful and practical thing for developers in Jerzyk's work is an online code smells catalogue that, when published, included the 56 code smells in the thesis. This catalogue is an accessible and searchable website. On the date of publication of this post, the catalogue contains 56 code smells.

In the catalogue code smells can be filtered by different classification criteria.

Example of filtering code smells in Jerzyk’s online catalogue.

For example, the screenshot above shows the result of filtering code smells that are OO Abusers and affect Interfaces which includes the Refused Bequest, Base Class depends on Subclass, and Inappropriate Static code smells.

A code smell in Jerzyk’s online catalogue.

For each code smell, the catalogue presents the following sections: Smell (discussion of the smell), Causation (possible causes of the code smell), Problems (problems that the smell can cause or design principles violates), Example (examples of minimal code that illustrate the possible symptoms of a code smell and show a possible solution), Refactoring (possible refactorings) and Sources (articles or books that have described this code smell). It also includes a box containing information about possible aliases of the code smell, the category to which it belongs according to the criteria of obstruction, occurrence and expanse, the related smells and its relationship to them, and the historical origin of the code smell.

Conclusions.

Since the code smell concept was coined in 1999, many new smells have appeared. Catalogues as flat lists of code smells catalogue are hard to remember, and don't help to highlight the relationships that exist between different code smells.

We have presented several code smells taxonomies that can help us see code smells from different points of view and relate them to each other according to different criteria: the problems they cause in the code, where they can be detected or the context that needs to be taken into account to detect them.

These meaningful groupings of code smells will help us understand and remember them better than flat catalogue lists.

Finally we’d like to highlight the recent work by Marcel Jerzyk which has not only proposed new smells and created a new multi-criteria taxonomy, but also has made available to us an online code smells catalogue in the form of an open-source repository and accessible and searchable website, which we believe can be very useful. useful and practical for both researchers and developers. I encourage you to take a look.

Acknowledgements.

I would like to thank my colleagues Fran Reyes, Antonio de La Torre, Miguel Viera and Alfredo Casado for reading the final drafts of this post and giving me feedback.

I’d also like to thank nikita for her photo.

References.

Books.

Refactoring: Improving the Design of Existing Code 1st edition 1999, Martin Fowler et al.
Refactoring: Improving the Design of Existing Code 2nd edition 2018, Martin Fowler et al.
Refactoring Workbook, William C. Wake
Refactoring to Patterns, Joshua Kerievsky
Five Lines of Code: How and when to refactor, Christian Clausen
The Programmer's Brain, Felienne Hermans

Papers.

A Taxonomy and an Initial Empirical Study of Bad Smells in Code, Mantyla et al, 2003.
Subjective evaluation of software evolvability using code smells: An empirical study, Mantyla et al, 2006.
A Taxonomy for "Bad Code Smells" , Mantyla et al, 2006.
Code Smells: A Comprehensive Online Catalog and Taxonomy, Marcel Jerzyk, 2022.
Code Smells: A Comprehensive Online Catalog and Taxonomy Msc. Thesis, Marcel Jerzyk, 2022.
Extending a Taxonomy of Bad Code Smells with Metrics, R. Marticorena et al, 2006

Photo by nikita in Pexels.

De taxonomías y catálogos de code smells

Manuel Rivero — Tue, 28 Feb 2023 17:32:46 +0000

Índice.

Introducción: refactoring y code smells.
Martin Fowler, Code Smells y Catálogos.
Taxonomías

Taxonomías de Wake 2003
Taxonomía de Mäntylä et al 2003
Taxonomía de Mäntylä et al 2006
Taxonomía de Jerzyk et al 2022

Catálogo de Code Smells
Catálogo online.

Conclusiones.
Agradecimientos.
Referencias.

Introducción: refactoring y code smells.

Refactorizar es una práctica que nos permite evolucionar un código de forma sostenible. Para poder hacerlo necesitamos, en primer lugar, ser capaces de reconocer el código problemático que necesita ser refactorizado.

Los code smells son descripciones de señales o síntomas que nos avisan de posibles problemas de diseño en nuestro código. Detectar estos problemas y eliminarlos tan pronto como uno se da cuenta de que hay algo mal es crucial.

El refactoring produce mejores resultados y es más barato si se hace con regularidad. Entre más tiempo permanezca sin refactorizar un código problemático, más se agravará su efecto y constreñirá el futuro desarrollo del código, contribuyendo directamente a la deuda técnica. Esta situación hará el código cada vez más difícil de mantener, lo que tendrá un impacto económico muy negativo, pudiendo incluso llegar, en el peor de los casos, a ser tan complicada que ya no se pueda seguir manteniendo el código.

Coste de introducir una feature en función del tiempo (imagen de The Three Values of Software de J. B. Rainsberger).

Otra consecuencia, a veces, no tan visible de la mala calidad del código es su efecto en los desarrolladores. Menos refactor, lleva a código menos mantenible, que nos lleva a tardar más tiempo en implementar nuevas funcionalidades, lo cuál nos mete más presión de tiempo, lo que nos lleva a testear menos, lo que nos lleva a refactorizar menos… Es un círculo vicioso que puede tener un efecto muy desmoralizador.

Círculo vicioso sin refactor.

Por tanto, entender y saber identificar los code smells nos da mucho poder porque seremos capaces de detectar problemas de diseño cuando aún son pequeños y están muy localizados en zonas concretas de nuestro código, y eso tendrá un efecto económico y anímico muy positivo.

El problema es que los code smells no se suelen entender demasiado bien. Esto es entendible porque sus definiciones son a veces abstractas, difusas y abiertas a interpretación. Algunos code smells parecen obvios, otros no tanto, y algunos pueden enmascarar a otros code smells.

Además, recordemos que los code smells son sólo síntomas de posibles problemas, y no garantías de problemas. Para complicarlo aún más, además de los posibles falsos positivos, existen grados en el problema que cada smell representa, trade-offs entre smells y contraindicaciones en sus refactorings (en ocasiones “el remedio puede ser peor que la enfermedad").

Por tanto, reconocer los code smells que señalan problemas de diseño, es decir, que no son falsos positivos, es una habilidad sutil, que requiere de experiencia y juicio. Adquirir esta habilidad, que a veces parece una especie de sentido arácnido, puede llevar tiempo.

En nuestra experiencia enseñando y acompañando a muchos equipos identificar code smells es una de las mayores barreras iniciales al refactoring. Muchos desarrolladores no se dan cuenta de los problemas del código que están generando cuando aún son pequeños. Lo que solemos ver es que no se dan cuenta hasta que los problemas son bastante grandes y/o se han combinado con otros problemas, propagándose por su base de código.

Catálogos de code smells.

En 1999 Fowler y Beck publicaron un catálogo de code smells en un capítulo del libro,
Refactoring: Improving the Design of Existing Code. Este catálogo contiene las descripciones de 22 code smells.

En 2018 Fowler publicó una segunda edición de su libro. En esta nueva edición hay una serie de cambios con respecto a la primera, principalmente en el catálogo de smells y el catálogo de refactorings (que detalla en Changes for the 2nd Edition of Refactoring).

Si nos centramos en los smells, lo que nos interesa en este post, los cambios son los siguientes:

Introduce cuatro code smells nuevos: Mysterious Name, Global Data, Mutable Data y Loops.
Elimina dos smells: Parallel Inheritance Hierarchies y Incomplete Library Class.
Renombra cuatro smells: Lazy Class pasa a ser Lazy Element, Long Method pasa a ser Long Function, Inappropriate Intimacy pasa a ser Insider Trading y Switch Statement pasa a ser Repeated Switches.

Quedan un total de 24 code smells, que aunque suelen tener nombres llamativos y memorables, son difíciles de recordar.

¿De qué manera podríamos entender mejor los code smells? ¿Cómo podríamos recordarlos más fácilmente?

En este post, y algunos posteriores, hablaremos de estrategias para profundizar, recordar y entender mejor los code smells.

Estrategias organizativas.

Organizar y manipular la información, verla desde diferentes puntos de vista, nos puede ayudar a entender y recordar mejor. Las taxonomías son un tipo de estrategia organizativa, que nos permite agrupar un material de estudio según su significado, creando agrupamientos significativos de información (“chunks"), que facilitarán el aprendizaje.

Como comentamos el catálogo de Fowler de 2018 es una lista plana que no proporciona ningún tipo de clasificación. Si bien, leyendo las descripciones de los de code smells, y las secciones de motivación de los diferentes refactorings podemos vislumbrar que algunos smells están más relacionados entre ellos que con otros smells, estas relaciones no se expresan de forma explícita y quedan difuminadas y dispersas en diferentes partes del libro.

El uso de taxonomías que clasifican code smells similares puede ser beneficioso para entenderlos mejor, recordarlos y reconocer las relaciones que existen entre ellos.

Taxonomías.

Ha habido diferentes intentos de clasificar los code smells agrupándolos según diferentes criterios. La clasificación más popular es la de Mäntylä et al 2006 pero no es la primera. A continuación mostraremos algunas que consideramos bastante interesantes.

Wake 2003.

Wake en su libro Refactoring Workbook de 2003 describe 9 nuevos code smells que no aparecían en el catálogo original de Fowler: Dead Code, Null Check, Special Case, Magic Number, Combinatorial Explosion, Complicated Boolean Expression, y tres relacionados con malos nombres: Type Embedded in Name, Uncommunicative Names e Inconsistent Names.

Wake clasifica explícitamente los code smells dividiéndolos primero en dos categorías amplias, Smells within Classes y Smells Between Classes, dependiendo, respectivamente, de si el code smell puede ser observado desde una clase o si se necesita considerar un contexto más amplio (varias clases). Cada una de estas categorías se divide en subcategorías que agrupan los code smells según en dónde se pueden detectar. Este criterio de clasificación, denominado más tarde “occurrence" por Jerzyk, responde a la pregunta: "¿dónde aparece este code smell?".

Siguiendo este criterio Wake encuentra las siguientes 10 subcategorías.

Dentro de la categoría de Smells within Classes estarían las siguientes subcategorías:

Measured Smells: code smells que pueden ser identificados fácilmente con simples métricas de longitud.
Names: code smells que crean confusión semántica y afectan a nuestra capacidad de crear modelos mentales que nos ayuden a comprender, recordar y razonar sobre el código.
Unnecessary Complexity: code smells relacionados con código innecesario que añade carga mental y complejidad. Código muerto, violaciones de YAGNI y complejidad accidental.
Duplication: la némesis de los desarrolladores. Estos code smells provocan que haya mucho más código que mantener (carga cognitiva y física), aumentan la propensión a errores y dificultan la comprensión del código.
Conditional Logic Smells: code smells que complican la lógica condicional haciendo difícil razonar sobre ella, dificultando el cambio y aumentando la propensión a cometer errores. Algunos de ellos son sucedáneos de mecanismos de la orientación a objetos.

Las subcategorías dentro de la categoría de Smells between Classes son:

Data: code smells en los que encontramos, o bien pseudo objetos (estructuras de datos sin comportamiento), o bien encontramos que falta alguna abstracción.
Inheritance: code smells relacionados con un mal uso de la herencia.
Responsibility: code smells relacionados con una mala asignación de responsabilidades.
Accommodating Change: code smells que se manifiestan cuando nos encontramos con mucha fricción al introducir cambios. Suelen estar provocados por combinaciones de otros code smells.
Library Classes: code smells relacionados con el uso de librerías de terceros.

Taxonomía de Wake (los nuevos smells aparecen en verde).

Wake presenta cada smell siguiendo un formato estándar con las siguientes secciones: Smell (el nombre y aliases), Síntomas (pistas que pueden ayudar a detectarlo), Causas (notas sobre cómo puede haberse generado), Qué Hacer (posibles refactorings), Beneficios (cómo mejorará el código al eliminarlo) y Contraindicaciones (falsos positivos y trade-offs). En algunos casos añade notas relacionando el code smell con principios de diseño que podrían ayudar a evitarlo.

El libro además contiene muchos ejercicios prácticos y tablas muy útiles (síntomas vs code smells, smells vs refactorings, refactorings inversos, etc) y ejercicios que relacionan los code smells con otros conceptos como principios de diseño o patrones de diseño.

Es un libro muy recomendable para profundizar en la disciplina de refactoring y entender mejor cuándo y por qué aplicar los refactorings que aparecen en el catálogo de Fowler.

Lo que enseñamos sobre code smells en nuestro curso sobre Code Smells & Refactoring se basa principalmente en esta clasificación de Wake aderezada con un poco de nuestra experiencia, aunque también hacemos referencia al resto de taxonomías de las que hablamos en este post.

Mäntylä et al 2003.

En ella los code smells se agrupan según el efecto que tienen en el código (el tipo de problema, lo que hacen difícil o las prácticas o principios que rompen). Jerzyk 2022 denomina "obstrucción" a este criterio de clasificación.

En la clasificación original de 2003 (A Taxonomy and an Initial Empirical Study of Bad Smells in Code) había 7 categorías de code smells: Bloaters, Object-Orientation Abusers, Change Preventers, Dispensables, Encapsulators, Couplers y Others.

Taxonomía de Mäntylä et al 2003.

Así es como definen cada una de las categorías (disculpen al traductor…):

Bloaters: “representan algo en el código que ha crecido tanto que ya no se puede manejar de forma efectiva.“
Object-Orientation Abusers: “esta categoría de smells se relaciona con casos en los que la solución no explota completamente las posibilidades del diseño orientado a objetos."
Change Preventers: "esta categoría hace referencia a estructuras de código que dificultan considerablemente cambiar el software."
Dispensables: “estos smells representan algo innecesario que debería ser eliminado del código."
Encapsulators: "tiene que ver con mecanismos de comunicación o encapsulación de datos."
Couplers: “estos smells representan casos de acoplamiento alto, lo cual va en contra de los principios de diseño orientado a objetos"
Others: “esta categoría contiene los code smells restantes (Comments e Incomplete Library Class) que no encajaban en ninguna de las categorías anteriores."

En el artículo, Mäntylä et al discuten los motivos por los que incluyeron cada smell en una determinada categoría y no otra, aunque admiten que algunos de ellos podrían ser clasificados en más de una categoría.

Mäntylä et al 2006.

En 2006 Mäntylä et al sacaron otro artículo (Subjective evaluation of software evolvability using code smells: An empirical study) en el que revisaron su clasificación original de 2003.

La diferencia de esta nueva versión es que elimina las categorías Encapsulators (moviendo los smells Message Chains y Middle Man a la categoría Couplers) y Others (Comments e Incomplete Library Class desaparecen de la taxonomía), y mueve el code smell Parallel Inheritance Hierarchies de la categoría de Object-Orientation Abusers a la categoría de Change Preventers.

Esta última versión de su taxonomía es la que se ha hecho más popular en internet (se puede encontrar en muchas webs, cursos y posts), probablemente debido a la mayor accesibilidad (facilidad de lectura) del resumen del artículo que aparece en la web que resume el artículo: A Taxonomy for "Bad Code Smells".

Taxonomía de Mäntylä et al 2006.

Lo interesante no es tanto la discusión de en qué categoría debe caer cada smell, sino el empezar a pensar qué un determinado smell puede tener diferentes tipos de efectos en el código y las relaciones entre estos efectos. De hecho, en posteriores clasificaciones desde el punto de vista del efecto de un smell en el código, no consideran ya las categorías como excluyentes, sino que, un mismo smell puede caer en varias categorías, ya que consideran que es más útil no perder la información de que un smell puede producir varios efectos.

Jerzyk et al 2022.

En 2022 Marcel Jerzyk publicó su tesis de master, Code Smells: A Comprehensive Online
Catalog and Taxonomy y un artículo con el mismo título. Su investigación sobre code smells tenía tres objetivos:

Proporcionar un catálogo público que pudiera ser útil como una base de conocimiento unificada tanto para investigadores como para desarrolladores.
Identificar todos los conceptos posibles que están siendo caracterizados como code smells y determinar posibles controversias sobre ellos.
Asignar propiedades apropiadas a los code smells con el fin de caracterizarlos.

Para conseguir estos objetivos realizaron una revisión de la literatura existente hasta ese momento sobre code smells, haciendo especial énfasis en las taxonomías de code smells.

En su tesis identifican y describen 56 code smells, de los cuales 16 son propuestas originales suyas, (recordemos que Wake describió 31 code smells y Fowler 24 en su última revisión). En la tesis de master de Jerzyk se pueden encontrar descripciones y discusiones sobre cada uno de estos 56 code smells.

Analizando los criterios de clasificación de las taxonomías propuestas anteriormente, Jerzyk encuentra tres criterios significativos para categorizar code smells:

Obstruction: Este es el criterio usado por Mäntylä et al para clasificar los smells en su taxonomía y el más popular. Este criterio nos informa sobre el tipo de problema que un code smell causa en el código (lo que hacen difícil o las prácticas o principios que rompen). En la tesis actualizan la taxonomía de Mäntylä, añadiendo tres nuevos grupos: Data Dealers, Functional Abusers y Lexical Abusers. A continuación presentamos un mapa mental que muestra la clasificación de los 56 code smells siguiendo únicamente este criterio.

Taxonomía de Jerzyk usando sólo el criterio de obstruction.

Expanse: Inspirado por la taxonomía de Wake, este criterio habla de si el code smell puede ser observado en un contexto reducido (dentro de una clase) o si se necesita considerar un contexto más amplio (entre varias clases). Las posibles categorías son Within Class y Between Classes.
Occurrence: También inspirado por la taxonomía de Wake, este criterio está relacionado con la localización donde (o el método por el cuál) se puede detectar un code smell. Las posibles categorías son Names, Conditional Logic, Message Calls, Unnecessary Complexity, Responsibility, Interfaces, Data, Duplication y Measured Smells.

A continuación presentamos una tabla con los 56 code smells clasificados por Jerzyk en su tesis usando los tres criterios comentados anteriormente:

Code Smell	Obstruction	Expanse	Occurrence
Long Method	Bloaters	Within	Measured Smells
Large Class	Bloaters	Within	Measured Smells
Long Parameter List	Bloaters	Within	Measured Smells
Primitive Obsession	Bloaters	Between	Data
Data Clumps	Bloaters	Between	Data
Null Check	Bloaters	Between	Conditional Logic
Oddball Solution	Bloaters	Between	Duplication
Required Setup/Teardown	Bloaters	Between	Responsibility
Combinatorial Explosion	Bloaters	Within	Responsibility
Parallel Inheritance Hierarchies	Change Preventers	Between	Responsibility
Divergent Change	Change Preventers	Between	Responsibility
Shotgun Surgery	Change Preventers	Between	Responsibility
Flag Argument	Change Preventers	Within	Conditional Logic
Callback Hell	Change Preventers	Within	Conditional Logic
Dubious Abstraction	Change Preventers	Within	Responsibility
Special Case	Change Preventers	Within	Conditional Logic
Feature Envy	Couplers	Between	Responsibility
Type Embedded In Name	Couplers	Within	Names
Indecent Exposure	Couplers	Within	Data
Fate over Action	Couplers	Between	Responsibility
Afraid to Fail	Couplers	Within	Responsibility
Binary Operator in Name	Couplers	Within	Names
Tramp Data	Data Dealers	Between	Data
Hidden Dependencies	Data Dealers	Between	Data
Global Data	Data Dealers	Between	Data
Message Chain	Data Dealers	Between	Message Calls
Middle Man	Data Dealers	Between	Message Calls
Insider Trading	Data Dealers	Between	Responsibility
Lazy Element	Dispensables	Between	Unnecessary Complexity
Speculative Generality	Dispensables	Within	Unnecessary Complexity
Dead Code	Dispensables	Within	Unnecessary Complexity
Duplicate Code	Dispensables	Within	Duplication
"What" Comments	Dispensables	Within	Unnecessary Complexity
Mutable Data	Functional Abusers	Between	Data
Imperative Loops	Functional Abusers	Within	Unnecessary Complexity
Side Effects	Functional Abusers	Within	Responsibility
Uncommunicative Name	Lexical Abusers	Within	Names
Magic Number	Lexical Abusers	Within	Names
Inconsistent Names	Lexical Abusers	Within	Names
Boolean Blindness	Lexical Abusers	Within	Names
Fallacious Comment	Lexical Abusers	Within	Names
Fallacious Method Name	Lexical Abusers	Within	Names
Complicated Boolean Expressions	Obfuscators	Within	Conditional Logic
Obscured Intent	Obfuscators	Between	Unnecessary Complexity
Vertical Separation	Obfuscators	Within	Measured Smells
Complicated Regex Expression	Obfuscators	Within	Names
Inconsistent Style	Obfuscators	Between	Unnecessary Complexity
Status Variable	Obfuscators	Within	Unnecessary Complexity
Clever Code	Obfuscators	Within	Unnecessary Complexity
Temporary Fields	O-O Abusers	Within	Data
Conditional Complexity	O-O Abusers	Within	Conditional Logic
Refused Bequest	O-O Abusers	Between	Interfaces
Alternative Classes with Different Interfaces	O-O Abusers	Between	Duplication
Inappropriate Static	O-O Abusers	Between	Interfaces
Base Class Depends on Subclass	O-O Abusers	Between	Interfaces
Incomplete Library Class	Other	Between	Interfaces

Algunos de los nombres en la tabla son diferentes de los que suelen aparecer en la literatura. Los cambios de nombre fueron debidos a la introducción de nombre más actualizados, como es el caso, por ejemplo, de Lazy Element o Insider Trading que antes se llamaban Lazy Class e Inappropriate Intimacy, respectivamente.

Hay varios smells que son nuevos. Algunos como Afraid to Fail, Binary Operator in Name, Clever Code, Inconsistent Style, y Status Variable, son ideas completamente nuevas.
Otros son conceptos ya existentes en la literatura pero que no se habían considerado en el contexto de los code smells: Boolean Blindness o Callback Hell. Tres de ellos proponen alternativas para code smells que están siendo cuestionados en la literatura: "What" Comment como alternativa para Comments, Fate over Action como alternativa para Data Class, e Imperative Loops como alternativa para Loops (ver la tesis de Jerzyk para profundizar en por qué estos code smells originales son discutibles). Otros generalizan otros conceptos problemáticos que han surgido en la literatura: Complicated Regex Expression, Dubious Abstraction, Fallacious Comment, Fallacious Method Name. Por último, un problema conocido (especialmente en el campo de la programación funcional), pero que no se ha considerado hasta ahora como code smell: Side Effects.

Una cosa super útil y práctica para desarrolladores que aporta el trabajo de Jerzyk es la creación de un catálogo online de code smells que incluía cuando se publicó los 56 code smells que aparecen en la tabla. Este catálogo es tanto un repositorio open-source como un sitio web accesible y buscable. Actualmente, en la fecha de publicación de este post, el catálogo contiene ya 86 code smells.

En el catálogo se pueden buscar los smells por diferentes criterios de clasificación.

Ejemplo de búsqueda en el catálogo online de code smells de Jerzyk.

Por ejemplo, esta captura de pantalla muestra el resultado de buscar code smells que sean OO Abusers y que afecten a Interfaces: Refused Bequest, Base Class depends on Subclass e Inappropriate Static.

Code smell en el catálogo online de Jerzyk.

Para cada smell el catálogo presentan las siguientes secciones: Smell (discusión del smell), Causation (posibles causas del code smell), Problems (problemas que el smell puede causar o principios de diseño que viola), Example (ejemplos de código mínimo que ilustran los posibles síntomas de un code smell y muestran una posible solución), Refactoring (posibles refactorings) y Sources (artículos o libros en los que se ha hablado de este code smell). También incluye un recuadro en el que aparece información sobre posibles aliases del code smell, la categoría a la que pertenece según los criterios de obstruction, occurrence y expanse, los smells relacionados y su relación con ellos, y el origen histórico del code smell.

Conclusiones.

Desde que se acuñó el concepto de code smell en 1999 han aparecido muchos nuevos smells. Las presentaciones planas en forma de catálogo son difíciles de recordar, y no ayudan a resaltar las relaciones que existen entre diferentes code smells.

Hemos presentado varias taxonomías de code smells que nos pueden ayudar a ver los code smells desde diferentes puntos de vista y relacionarlos unos con otros según diferentes criterios: los problemas que producen en el código, dónde se detectan o el contexto que es necesario tener en cuenta para detectarlos.

Estos agrupamientos significativos de code smells nos ayudarán a entenderlos y a recordarlos mejor que las listas planas de los catálogos.

Por último queremos destacar el reciente trabajo de Marcel Jerzyk que no sólo ha propuesto nuevos smells y ha creado una nueva taxonomía multicriterio, sino que además ha puesto a nuestra disposición un catálogo online de code smells en forma de repositorio open-source y sitio web accesible y buscable, que creemos que puede resultar muy útil y práctico tanto para investigadores como para desarrolladores. Los animo a echarle un vistazo.

Agradecimientos.

Me gustaría darle las gracias a mis colegas Fran Reyes, Antonio de La Torre, Miguel Viera y Alfredo Casado por leer los borradores finales de este post y darme feedback. También quería agradecer a nikita por su foto.

Referencias.

Libros.

Refactoring: Improving the Design of Existing Code 1st edition 1999, Martin Fowler et al.
Refactoring: Improving the Design of Existing Code 2nd edition 2018, Martin Fowler et al.
Refactoring Workbook, William C. Wake
Refactoring to Patterns, Joshua Kerievsky
Five Lines of Code: How and when to refactor, Christian Clausen
The Programmer's Brain, Felienne Hermans

Artículos.

A Taxonomy and an Initial Empirical Study of Bad Smells in Code, Mantyla et al, 2003.
Subjective evaluation of software evolvability using code smells: An empirical study, Mantyla et al, 2006.
A Taxonomy for "Bad Code Smells" , Mantyla et al, 2006.
Code Smells: A Comprehensive Online Catalog and Taxonomy, Marcel Jerzyk, 2022.
Code Smells: A Comprehensive Online Catalog and Taxonomy Msc. Thesis, Marcel Jerzyk, 2022.
Extending a Taxonomy of Bad Code Smells with Metrics, R. Marticorena et al, 2006

Foto de nikita in Pexels

Simplifying jest stubs using jest-when

Manuel Rivero — Fri, 10 Feb 2023 19:40:32 +0000

In a recent deliberate practice session with some developers from Audiense (with whom we’re doing the Codesai’s Practice Program twice a month), we were solving the Unusual Spending Kata
in JavaScript.

While test-driving the UnusualSpendingDetector class we found that writing stubs using plain jest can be a bit hard. The reason is that jest does not match mocked function arguments, so to create stubbed responses for particular values of the arguments we are forced to introduce logic in the tests.

Have a look at a fragment of the tests we wrote for UnusualSpendingDetector class:

import {UnusualSpendingDetector} from "../src/UnusualSpendingDetector";

describe('UnusualSpendingDetector', () => {
  const currentMonth = '2020-02';
  const previousMonth = '2020-01';
  const userId = '1234';
  let paymentsRepository;
  let detector;

  beforeEach(() => {
    const calendar = {
      getCurrentMonth: () => currentMonth,
      getPreviousMonth: () => previousMonth,
    };
    paymentsRepository = {
      find: jest.fn()
    }
    detector = new UnusualSpendingDetector(paymentsRepository, calendar);
  });

  // more tests ...

  test(
    'detects an unusual spending when spending for a category in consecutive months grew 50% or more', 
    () => {
      const currentMonthPayments = [payment('food', 200, currentMonth)];
      const previousMonthPayments = [payment('food', 100, previousMonth)];
      paymentsRepositoryWillReturn(currentMonthPayments, previousMonthPayments);

      const unusualSpendings = detector.detect(userId);

      expect(unusualSpendings).toEqual([unusualSpending('food', 200)]);
  });

  // more tests ...

  function payment(category, amount, month) {
    return {category, amount, month};
  }

  function unusualSpending(category, amount) {
    return {category, amount};
  }

  function paymentsRepositoryWillReturn(currentMonthPayments, previousMonthPayments) {
    paymentsRepository.find.mockImplementation((userId, month) => {
      if (month === currentMonth) {
        return currentMonthPayments;
      } else if (month === previousMonth) {
        return previousMonthPayments;
      }
    });
  }
});

Notice the paymentsRepositoryWillReturn helper function, we extracted it to remove duplication in the tests. In this function we had to add explicit checks to see whether the arguments of a call match some given values. It reads more or less ok, but we were not happy with the result because we were adding logic in test code (see all the test cases).

Creating these stubs can be greatly simplified using a library called jest-when which helps to write stubs for specifically matched mocked function arguments. Have a look at the same fragment of the tests we wrote for UnusualSpendingDetector class now using jest-when:

import {UnusualSpendingDetector} from "../src/UnusualSpendingDetector";
import {when} from 'jest-when';

describe('UnusualSpendingDetector', () => {
  const currentMonth = '2020-02';
  const previousMonth = '2020-01';
  const userId = '1234';
  let paymentsRepository;
  let detector;

  beforeEach(() => {
    const calendar = {
      getCurrentMonth: () => currentMonth,
      getPreviousMonth: () => previousMonth,
    };
    paymentsRepository = {
      find: jest.fn()
    }
    detector = new UnusualSpendingDetector(paymentsRepository, calendar);
  });

  // more tests ...

  test('detects an unusual spending when spending for a category in consecutive months grew 50% or more', () => {
    when(paymentsRepository.find).calledWith(userId, currentMonth)
      .mockReturnValue([payment('food', 200, currentMonth)]);
    when(paymentsRepository.find).calledWith(userId, previousMonth)
      .mockReturnValue([payment('food', 100, previousMonth)]);

    const unusualSpendings = detector.detect(userId);

    expect(unusualSpendings).toEqual([unusualSpending('food', 200)]);
  });

  // more tests ...

  function payment(category, amount, month) {
    return {category, amount, month};
  }

  function unusualSpending(category, amount) {
    return {category, amount};
  }
});

Notice how the paymentsRepositoryWillReturn helper function is not needed anymore, and how the fluent interface of jest-when feels nearly like canonical jest syntax.

We think that using jest-when is less error prone than having to add logic to write your own stubs with plain jest, and it’s as readable as or more than using only jest (see all the refactored test cases).

To improve readability we played a bit with a functional builder. This is the same fragment of the tests we wrote for UnusualSpendingDetector class now using a functional builder over jest-when:

import {UnusualSpendingDetector} from "../src/UnusualSpendingDetector";
import {stub} from "./PaymentsRepositoryHelper";

describe('UnusualSpendingDetector', () => {
  const currentMonth = '2020-02';
  const previousMonth = '2020-01';
  const userId = '1234';
  let paymentsRepository;
  let detector;
  let knowingThat;

  beforeEach(() => {
      paymentsRepository = {
        find: jest.fn()
      };
      knowingThat = stub(paymentsRepository);
      const calendar = {
        getCurrentMonth: () => currentMonth,
        getPreviousMonth: () => previousMonth,
      };
      detector = new UnusualSpendingDetector(paymentsRepository, calendar);
    }
  );

  // more tests ...

  test(
    'detects an unusual spending when spending for a category in consecutive months grew 50% or more', 
    () => {
      knowingThat().inMonth(currentMonth).userWith(userId).hasPaid(
        [payment('food', 200, currentMonth)])
        .andThat().inMonth(previousMonth).userWith(userId).hasPaid(
        [payment('food', 100, previousMonth)]);

      const unusualSpendings = detector.detect(userId);

      expect(unusualSpendings).toEqual([unusualSpending('food', 200)]);
  });

  // more tests ...

  function payment(category, amount, month) {
    return {category, amount, month};
  }

  function unusualSpending(category, amount) {
    return {category, amount};
  }
});

where the PaymentsRepositoryHelper is as follows:

import {when} from "jest-when";

export function stub(paymentsRepository) {
  const monthPaymentsBuilder = {
    inMonth: inMonth
  };

  return knowingThat;

  function knowingThat() {
    return monthPaymentsBuilder;
  }

  function inMonth(month) {
    return {
      userWith: (userId) => {
        return {
          hasPaid: (payments) => {
            when(paymentsRepository.find).calledWith(userId, month)
              .mockReturnValue(payments);
            return {
              andThat: knowingThat
            };
          }
        };
      }
    };
  }
}

In this last version we were just playing a bit with the code in order to find a way to configure the stubs both in terms of the domain and without free variables (have a look at all the test cases using this builder). In any case, we think that the previous version using jest-when was already good enough.

Probably you already knew jest-when, if not, give it a try. We think it can help you to write simpler stubs if you're using jest.

"Split or condition in if" refactoring in TypeScript

Manuel Rivero — Mon, 30 Jan 2023 18:45:16 +0000

In a previous post we documented the Split or condition in if refactoring. This refactoring works fine for most languages. Unfortunately, it may occasionally present some problems in TypeScript because of how its compiler works.

After applying the Split or condition in if refactoring, the resulting code will contain duplication, and there may be obsolete if statements containing either unreachable code or code that is always executed, that we will clean.

If the resulting code contains any if statement whose boolean condition always evaluates to false, the TypeScript compiler will emit a TS2367 error, even though the code is functionally correct. We show an example of this problem in the following figure.

Compiler error when a boolean expression always evaluates to false.

In these cases, the code would not compile again, and as a consequence, the test won’t pass until we remove all the boolean conditions that always evaluate to false or indicate the compiler to ignore them using @ts-ignore or @ts-expect-error. This might be ok if there are only a few TS2367 errors. If not, cleaning those expressions will take too much time editing without having any feedback from the tests.

Revised mechanics for TypeScript.

From TypeScript 3.7 onward, we can add the @ts-nocheck rule to the top of TypeScript files to disable semantic checks (before that version, @ts-nocheck only worked for JavaScript files).

Using TypeScript’s @ts-nocheck rule will help us getting a shorter feedback cycle again, when cleaning redundant code resulting from applying the Split or condition in if refactoring that contains TS2367 errors.

We only need to slightly modify the original Split or condition in if refactoring mechanics:

Add the @ts-nocheck rule to the top of TypeScript files to disable semantic checks.
Apply the original Split or condition in if refactoring .
Remove obsolete if statements in small steps while being able to get feedback from the tests at any time.
Remove the @ts-nocheck rule to enable semantic checks again.

In the following video, we show an example of how using @ts-nocheck allows us to be able to keep refactoring the code in small steps after applying the Split or condition in if refactoring.

Conclusion.

We described a problem that TypeScript developers will find when the code resulting after applying the Split or condition in if refactoring contains any boolean condition that always evaluates to false which will produce TS2367 errors, and we provided a way to fix it. Following revised Split or condition in if refactoring mechanics that we proposed, you will be able to keep refactoring in small steps to remove unreachable code left after applying the Split or condition in if refactoring even when there are TS2367 errors.

We hope this revision of the Split or condition in if refactoring would be useful for TypeScript developers.

Acknowledgements.

I’d like to thank Fran Reyes for recording the Split or condition in if refactoring video in this post, and for giving me feedback on the contents of this post.

I’d also like to thank Yuliya Kota for her photo.

References

Articles

Photo by Yuliya Kota.

Example of role tests in JavaScript with Jest

Manuel Rivero — Mon, 23 Jan 2023 16:05:05 +0000

In this post we’ll show our last example applying the concept of role tests, this time in JavaScript using Jest. Have a look at our previous posts on this topic.

This example comes from a deliberate practice session we did recently with some developers from Audiense with whom we’re doing Codesai’s Practice Program in JavaScript twice a month.

Similar to what we did in our previous example of role tests in Java, we wrote the following tests to develop two different implementations of the TransactionsRepository port while solving the Bank Kata: the InMemoryTransactionsRepository and the NodePersistTransactionRepository.

These are their tests, respectively:

import {InMemoryTransactionRepository} from "../../src/inMemoryTransactionRepository";
import {aDeposit, aWithdrawal, date} from "../helpers";

describe('inMemory transaction repository', () => {
  let repository;

  beforeEach(() => {
    repository = new InMemoryTransactionRepository();
  });

  test('should save a transaction', () => {
    const transaction = aDeposit(100, date(2022, 6, 8));

    repository.save(transaction);

    expect(repository.getAll()).toStrictEqual([transaction]);
  });

  test('should add a transaction', () => {
    const transaction = aDeposit(100, date(2022, 6, 8));
    writeTransactions([transaction]);
    const newTransaction = aDeposit(200, date(2022, 6, 9));

    repository.save(newTransaction);

    expect(repository.getAll()).toStrictEqual([transaction, newTransaction]);
  });

  test('should get empty list of transactions if none saved', () => {
    const retrievedTransactions = repository.getAll();

    expect(retrievedTransactions).toStrictEqual([])
  });

  test('should get all transactions', () => {
    const transactions = [
      aDeposit(100, date(2022, 6, 8)),
      aWithdrawal(200, date(2022, 6, 9))
    ];
    writeTransactions(transactions);

    const retrievedTransactions = repository.getAll();

    expect(retrievedTransactions).toStrictEqual(transactions)
  });

  function writeTransactions(transactions) {
    transactions.forEach(t => repository.save(t));
  }
});

import crypto from "crypto";
import fs from 'fs';
import {aDeposit, aWithdrawal, date} from "../helpers";
import {NodePersistTransactionRepository} from "../../src/nodePersistTransactionRepository";

describe('node-persist transaction repository', () => {
  let repository;
  const filePath = `./tmp/${md5('transactions')}`;

  beforeEach(() => {
    fs.rmSync('./tmp', {force: true, recursive: true});
    fs.mkdirSync('./tmp');
    repository = new NodePersistTransactionRepository('./tmp');
  });

  afterEach(() => {
    fs.rmSync('./tmp', {force: true, recursive: true});
  });

  test('should save a transaction', () => {
    const transaction = aDeposit(100, date(2022, 6, 8));

    repository.save(transaction);

    expect(readTransactions()).toStrictEqual([transaction]);
  });

  test('should add a transaction', () => {
    const transaction = aDeposit(100, date(2022, 6, 8));
    writeTransactions([transaction]);
    const newTransaction = aDeposit(200, date(2022, 6, 9));

    repository.save(newTransaction);

    expect(readTransactions()).toStrictEqual([transaction, newTransaction]);
  });

  test('should get empty list of transactions if none saved', () => {
    const retrievedTransactions = repository.getAll();

    expect(retrievedTransactions).toStrictEqual([])
  });

  test('should get all transactions', () => {
    const transactions = [
      aDeposit(100, date(2022, 6, 8)),
      aWithdrawal(200, date(2022, 6, 9))
    ];
    writeTransactions(transactions);

    const retrievedTransactions = repository.getAll();

    expect(retrievedTransactions).toStrictEqual(transactions)
  });

  function readTransactions() {
    let content = fs.readFileSync(filePath, {encoding: 'utf-8'});
    return JSON.parse(content).value.map(t => ({...t, date: new Date(t.date)}));
  }

  function writeTransactions(transactions) {
    fs.writeFileSync(filePath, JSON.stringify({
      key: 'transactions',
      value: transactions
    }), {encoding: 'utf-8'});
  }

  function md5(key) {
    return crypto.createHash('md5').update(key).digest('hex');
  }
});

As what happened in our previous post, both tests contain the same test cases since both tests document and protect the contract of the same role, TransactionsRepository, which InMemoryTransactionsRepository and NodePersistTransactionRepository implement.

Again we’ll use the concept of role tests to remove that duplication, and make the contract of the role we are implementing more explicit.

Although Jest does not have something equivalent or similar to the RSpec’s shared examples functionality we used in our previous example in Ruby, we can get a very similar result by composing functions.

First, we wrote the behavesLikeATransactionRepository function. This function contains all the test cases that document the role and protect its contract, and receives as a parameter a testContext object containing methods for all the operations that will vary in the different implementations of this integration test.

import {aDeposit, aWithdrawal, date} from "../helpers";

export function behavesLikeATransactionRepository(testContext) {
  return () => describe('behaves like a transaction repository', () => {
    let repository;
    beforeEach(() => {
      testContext.init();
      repository = testContext.getInstance();
    });

    afterEach(() => {
      testContext.clean();
    });

    test('should save a transaction', () => {
      const transaction = aDeposit(100, date(2022, 6, 8));

      repository.save(transaction);

      expect(testContext.readTransactions()).toStrictEqual([transaction]);
    });

    test('should add a transaction', () => {
      const transaction = aDeposit(100, date(2022, 6, 8));
      testContext.writeTransactions([transaction]);
      const newTransaction = aDeposit(200, date(2022, 6, 9));

      repository.save(newTransaction);

      expect(testContext.readTransactions()).toStrictEqual([transaction, newTransaction]);
    });

    test('should get empty list of transactions if none saved', () => {
      const retrievedTransactions = repository.getAll();

      expect(retrievedTransactions).toStrictEqual([])
    });

    test('should get all transactions', () => {
      const transactions = [
        aDeposit(100, date(2022, 6, 8)),
        aWithdrawal(200, date(2022, 6, 9))
      ];
      testContext.writeTransactions(transactions);

      const retrievedTransactions = repository.getAll();

      expect(retrievedTransactions).toStrictEqual(transactions)
    });
  });
}

Notice that in the case of Jest we are using composition, whereas we used inheritance in the case of Junit.

Then, we called the behavesLikeATransactionRepository function from the previous tests and implemented a particular version of the methods of the testContext object for each test.

This is the new code of InMemoryTransactionsRepositoryTest:

import {behavesLikeATransactionRepository} from "./transactionRepository.role";
import {InMemoryTransactionRepository} from "../../src/inMemoryTransactionRepository";

let repository;

describe('inMemory transaction repository', 
         behavesLikeATransactionRepository({
  init() {
    repository = new InMemoryTransactionRepository();
  },
  clean() {
  },
  getInstance() {
    return repository;
  },
  readTransactions() {
    return repository.getAll();
  },
  writeTransactions(transactions) {
    transactions.forEach(t => repository.save(t));
  }
}));

And this is the new code of NodePersistTransactionRepository after the refactoring:

import crypto from "crypto";
import fs from 'fs';
import {NodePersistTransactionRepository} from "../../src/nodePersistTransactionRepository";
import {behavesLikeATransactionRepository} from "./transactionRepository.role";

const filePath = `./tmp/${md5('transactions')}`;

describe('node-persist transaction repository', 
          behavesLikeATransactionRepository({
  init() {
    fs.rmSync('./tmp', {force: true, recursive: true});
    fs.mkdirSync('./tmp');
  },
  clean() {
    fs.rmSync('./tmp', {force: true, recursive: true});
  },
  getInstance() {
    return new NodePersistTransactionRepository('./tmp');
  },
  readTransactions() {
    let content = fs.readFileSync(filePath, {encoding: 'utf-8'});
    return JSON.parse(content).value.map(t => ({...t, date: new Date(t.date)}));
  },
  writeTransactions(transactions) {
    fs.writeFileSync(filePath, JSON.stringify({
      key: 'transactions',
      value: transactions
    }), {encoding: 'utf-8'});
  }
}));

function md5(key) {
  return crypto.createHash('md5').update(key).digest('hex');
}

This new version of the tests not only reduces duplication, but also makes explicit and protects the behaviour of the TransactionsRepository role. It also makes less error prone the process of adding a new implementation of TransactionsRepository because just by using the behavesLikeATransactionRepository function, you’d get a checklist of the behaviour you need to implement in order to ensure substitutability, i.e., to ensure the Liskov Substitution Principle is not violated.

These role tests using composition are also more readable than the Junit ones, in my opinion at least :)

Acknowledgements.

I’d like to thank Audiense’s deliberate practice group for working with us on this kata, and my colleague Rubén Díaz for co-facilitating the practice sessions with me.

Thanks to my Codesai colleagues for reading the initial drafts and giving me feedback, and to Elina Sazonova for the picture.

References.

Photo from Elina Sazonova in Pexels

Example of role tests in Java with Junit

Manuel Rivero — Wed, 18 Jan 2023 17:14:03 +0000

I’d like to continue with the topic of role tests that we wrote about in a previous post, by showing an example of how it can be applied in Java to reduce duplication in your tests.

This example comes from a deliberate practice session I did recently with some people from Women Tech Makers Barcelona with whom I’m doing Codesai’s Practice Program in Java twice a month.

Making additional changes to the code that resulted from solving the Bank Kata we wrote the following tests to develop two different implementations of the TransactionsRepository port: the InMemoryTransactionsRepository and the FileTransactionsRepository.

These are their tests, respectively:

package bank.tests.integration;

// some ommited imports...

public class InMemoryTransactionsRepositoryTest {

  private TransactionsRepository repository;
  private List<Transaction> initialTransactions;

  @Before
  public void setup() throws ParseException {
    repository = new InMemoryTransactionsRepository();
    initialTransactions = Arrays.asList(
        aTransaction().withDeposit(100).on("10/10/2021").build(),
        aTransaction().withWithdrawal(50).on("15/10/2021").build());
    prepareData(initialTransactions);
  }

  @Test
  public void a_transaction_can_be_saved() throws ParseException {
    Transaction transaction = aTransaction().withDeposit(500).on("25/10/2021").build();

    repository.save(transaction);

    assertThat(repository.retrieveAll(), is(addTo(initialTransactions, transaction)));
  }

  @Test
  public void transactions_can_be_retrieved() {
    assertThat(repository.retrieveAll(), is(initialTransactions));
  }

  private void prepareData(List<Transaction> transactions) {
    for (Transaction transaction : transactions) {
      repository.save(transaction);
    }
  }
}

package bank.tests.integration;

// some ommited imports...

public class FileTransactionsRepositoryTest {
  private static final String TRANSACTIONS_FILE = "src/test/resources/initial_transactions.txt";

  private TransactionsFile transactionsFile;
  private TransactionsRepository repository;
  private List<Transaction> initialTransactions;

  @Before
  public void setup() throws ParseException {
    transactionsFile = new TransactionsFile(TRANSACTIONS_FILE);
    repository = new FileTransactionsRepository(TRANSACTIONS_FILE);
    initialTransactions = Arrays.asList(
        aTransaction().withDeposit(100).on("10/10/2021").build(),
        aTransaction().withWithdrawal(50).on("15/10/2021").build());
    prepareData(initialTransactions);
  }

  @Test
  public void a_transaction_can_be_saved() throws ParseException {
    Transaction transaction = aTransaction().withDeposit(500).on("25/10/2022").build();

    repository.save(transaction);

    assertThat(repository.retrieveAll(), is(addTo(initialTransactions, transaction)));
  }

  @Test
  public void transactions_can_be_retrieved() {
    assertThat(repository.retrieveAll(), is(initialTransactions));
  }

  private void prepareData(List<Transaction> transactions) {
    try {
      transactionsFile.clean();
      transactionsFile.append(transactions);
    } catch (IOException e) {
      System.err.println("Error preparing transaction data in tests: " + e);
    }
  }
}

As you can see both tests contain the same test cases: a_transaction_can_be_saved and transactions_can_be_retrieved but their implementations are different for each class. This makes sense because both implementations implement the same role, (see our previous post to learn how this relates to Liskov Substitution Principle).

We can make this fact more explicit by using role tests. In this case, Junit does not have something equivalent or similar to the RSpec’s shared examples functionality we used in our previous example in Ruby. Nonetheless, we can apply the Template Method pattern to write the role test, so that we remove the duplication, and more importantly make the contract we are implementing more explicit.

To do that we created an abstract class, TransactionsRepositoryRoleTest. This class contains the tests cases that document the role and protect its contract (a_transaction_can_be_saved and transactions_can_be_retrieved) and defines hooks for the operations that will vary in the different implementations of this integration test
(prepareData, readAllTransactions and createRepository):

package bank.tests.integration.roles;

// some ommited imports...

public abstract class TransactionsRepositoryRoleTest {
  protected TransactionsRepository repository;
  private List<Transaction> initialTransactions;

  @Before
  public void setup() throws ParseException {
    repository = createRepository();
    initialTransactions = Arrays.asList(
        aTransaction().withDeposit(100).on("10/10/2021").build(),
        aTransaction().withWithdrawal(50).on("15/10/2021").build());
    prepareData(initialTransactions);
  }

  @Test
  public void a_transaction_can_be_saved() throws ParseException {
    Transaction transaction = aTransaction().withDeposit(500).on("25/10/2021").build();

    repository.save(transaction);

    assertThat(readAllTransactions(), is(add(initialTransactions, transaction)));
  }

  @Test
  public void transactions_can_be_retrieved() {
    assertThat(repository.retrieveAll(), is(initialTransactions));
  }

  protected abstract TransactionsRepository createRepository();

  protected abstract List<Transaction> readAllTransactions();

  protected abstract void prepareData(List<Transaction> transactions);

  private List<Transaction> add(List<Transaction> transactions, Transaction transaction) {
    List<Transaction> result = new ArrayList<>(transactions);
    result.add(transaction);
    return result;
  }
}

Then we made the previous tests extend TransactionsRepositoryRoleTest and implemented the hooks.

This is the new code of InMemoryTransactionsRepositoryTest:

package bank.tests.integration;

// some ommited imports...

public class InMemoryTransactionsRepositoryTest extends TransactionsRepositoryRoleTest {

  @Override
  protected TransactionsRepository createRepository() {
    return new InMemoryTransactionsRepository();
  }

  @Override
  protected void prepareData(List<Transaction> initialTransactions) {
    for (Transaction transaction : initialTransactions) {
      repository.save(transaction);
    }
  }

  @Override
  protected List<Transaction> readAllTransactions() {
    return repository.retrieveAll();
  }
}

And this is the new code of FileTransactionsRepositoryTest after the refactoring:

package bank.tests.integration;

// some ommited imports...

public class FileTransactionsRepositoryTest extends TransactionsRepositoryRoleTest {
  private static final String TRANSACTIONS_FILE = "src/test/resources/initial_transactions.txt";
  private final TransactionsFile transactionsFile;

  public FileTransactionsRepositoryTest() {
    transactionsFile = new TransactionsFile(TRANSACTIONS_FILE);
  }

  @Override
  protected TransactionsRepository createRepository() {
    return new FileTransactionsRepository(TRANSACTIONS_FILE);
  }

  @Override
  protected void prepareData(List<Transaction> transactions) {
    try {
      transactionsFile.clean();
      transactionsFile.append(transactions);
    } catch (IOException e) {
      System.err.println("Error preparing transaction data in tests: " + e);
    }
  }

  @Override
  protected List<Transaction> readAllTransactions() {
    List<Transaction> transactions = new ArrayList<>();
    try {
      transactions = transactionsFile.readTransactions();
    } catch (IOException | ParseException e) {
      System.err.println("Error reading transaction data in test: " + e);
    }
    return transactions;
  }
}

This new version of the tests not only reduces duplication, but also makes explicit and protects the behaviour of the TransactionsRepository role. It also makes less error prone the process of adding a new implementation of TransactionsRepository because just by extending the TransactionsRepositoryRoleTest, you’d get a checklist of the behaviour you need to implement to ensure substitutability, i.e., to ensure the Liskov Substitution Principle is not violated.

Have a look at this Jason Gorman’s repository to see another example that applies the same technique.

In a future post we’ll show how we can do the same in JavaScript using Jest.

Acknowledgements.

I’d like to thank the WTM study group, and especially Inma Navas and Laura del Toro for practising with this kata together.

Thanks to my Codesai colleagues, Inma Navas and Laura del Toro for reading the initial drafts and giving me feedback, and to Esranur Kalay for the picture.

References.

Photo from Esranur Kalay
in Pexels

Simple example of property-based testing

Manuel Rivero — Tue, 10 Jan 2023 17:40:32 +0000

Introduction.

We were recently writing tests to characterise a legacy code at a client that was being used to encrypt and decrypt UUIDs using a cipher algorithm. We have simplified the client's code to remove some distracting details and try to highlight the key ideas we’d like to transmit.

In this post we’ll show how we used parameterized tests to test the encryption and decryption functions, and how we applied property-based testing to explore possible edge cases using a very useful pattern to discover properties.

Finally, we’ll discuss the resulting tests.

Testing the encryption and decryption functions.

We started by writing examples to fixate the behaviour of the encryption function. Notice how we used parameterized tests^[1] to avoid the duplication that having a different test for each example would have caused:

package com.ads.domain;

import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

import java.util.List;

import static java.util.Arrays.asList;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;

public class UuidCipherTest {

  @ParameterizedTest
  @MethodSource("generateCipheringUuidExamples")
  void encrypt_ids(String originalUuid, String cypheredUuid) {
    UuidCipher uuidCipher = new UuidCipher();
    assertThat(uuidCipher.encrypt(originalUuid), is(cypheredUuid));
  }

  private static List<Arguments> generateCipheringUuidExamples() {
    return asList(
      Arguments.of("3a924d98-93d6-4c58-b358-0c569818aa9f", "9400-b382-3a924dc2-c569818aac9-4c82"),
      Arguments.of("3a924d98-aaaa-4c58-b358-0c569818aa9f", "aad4-b382-3a924dc2-c569818aac9-4c82"),
      Arguments.of("3a924d98-93d6-4c58-b358-999999999999", "9400-b382-3a924dc2-9999999999c3-4c82"));
  }
}

Then we wrote more parameterized tests for the decryption function. Since the encryption and decryption functions are inverses of each other, we could use the same examples that we had used for the encryption function. Notice how the roles of being input and expected output change for the parameters of the new test.

package com.ads.domain;

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

import java.util.List;
import java.util.UUID;

import static java.util.Arrays.asList;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;

public class UuidCipherTest {

  private UuidCipher uuidCipher;

  @BeforeEach
  void setUp() {
    uuidCipher = new UuidCipher();
  }

  @ParameterizedTest
  @MethodSource("generateCipheringUuidExamples")
  void encrypt_ids(String originalUuid, String cypheredUuid) {
    assertThat(uuidCipher.encrypt(originalUuid), is(cypheredUuid));
  }

  @ParameterizedTest
  @MethodSource("generateCipheringUuidExamples")
  void decrypt_ids(String originalUuid, String cypheredUuid) {
    assertThat(uuidCipher.decrypt(cypheredUuid), is(originalUuid));
  }

  private static List<Arguments> generateCipheringUuidExamples() {
    return asList(
      Arguments.of("3a924d98-93d6-4c58-b358-0c569818aa9f", "9400-b382-3a924dc2-c569818aac9-4c82"),
      Arguments.of("3a924d98-aaaa-4c58-b358-0c569818aa9f", "aad4-b382-3a924dc2-c569818aac9-4c82"),
      Arguments.of("3a924d98-93d6-4c58-b358-999999999999", "9400-b382-3a924dc2-9999999999c3-4c82"));
  }

  private UuidCipher createUuidCipher() {
    return new UuidCipher();
  }
}

Exploring edge cases.

You might wonder why we wanted to explore edge cases. Weren’t the parameterized tests enough to characterise this legacy code?

Even though, the parameterized tests that we wrote for both functions were producing a high test coverage, coverage only “covers” code that is already there. We were not sure if there could be any edge cases, that is, inputs for which the encryption and decryption functions might not behave correctly. We’ve found edge cases in the past even in code with 100% unit test coverage.

Finding edge cases is hard work which sometimes might require exploratory testing by specialists. It would be great to automatically explore some behaviour to find possible edge cases so we don’t have to find them ourselves or some QA specialists. In some cases, we can leverage property-based testing to do that exploration for us^[2].

One of the most difficult parts of using property-based testing is finding out what properties we should use. Fortunately, there are several approaches or patterns for discovering adequate properties to apply property-based testing to a given problem. Scott Wlaschin wrote a great article in which he explains several of those patterns^[3].

It turned out that the problem we were facing matched directly to one of the patterns described by Wlaschin, the one he calls “There and back again”(also known as “Round-tripping” or “Symmetry” pattern).

According to Wlaschin “There and back again” properties “are based on combining an operation with its inverse, ending up with the same value you started with”.

As we said before, in our case the decryption and encryption functions were inverses of each other so the “There and back again” pattern was likely to lead us to a useful property.

Once we knew which property to use it was very straightforward to add a property-based test for it. We used the jqwik library. We like it because it has very good documentation and it is integrated with Junit.

Using jqwik functions we wrote a generator of UUIDs (have a look at the documentation on how to generate customised parameters), we then wrote the decrypt_is_the_inverse_of_encrypt property:

package com.ads.domain;

import net.jqwik.api.*;
import net.jqwik.api.lifecycle.BeforeProperty;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

import java.util.List;
import java.util.UUID;

import static java.util.Arrays.asList;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;

public class UuidCipherTest {

  private UuidCipher uuidCipher;

  @BeforeEach
  @BeforeProperty
  void setUp() {
    uuidCipher = new UuidCipher();
  }

  @ParameterizedTest
  @MethodSource("generateCipheringUuidExamples")
  void encrypt_ids(String originalUuid, String cypheredUuid) {
    assertThat(uuidCipher.encrypt(originalUuid), is(cypheredUuid));
  }

  @ParameterizedTest
  @MethodSource("generateCipheringUuidExamples")
  void decrypt_ids(String originalUuid, String cypheredUuid) {
    assertThat(uuidCipher.decrypt(cypheredUuid), is(originalUuid));
  }

  @Property
  void decrypt_is_the_inverse_of_encrypt(@ForAll("uuids") String originalUuid) {
    assertThat(uuidCipher.decrypt(uuidCipher.encrypt(originalUuid)), is(originalUuid));
  }

  @Provide
  Arbitrary<String> uuids() {
    return Arbitraries
      .longs()
      .tuple2()
      .map(longs -> new UUID(longs.get1(), longs.get2()).toString());
  }

  private static List<Arguments> generateCipheringUuidExamples() {
    return asList(
      Arguments.of("3a924d98-93d6-4c58-b358-0c569818aa9f", "9400-b382-3a924dc2-c569818aac9-4c82"),
      Arguments.of("3a924d98-aaaa-4c58-b358-0c569818aa9f", "aad4-b382-3a924dc2-c569818aac9-4c82"),
      Arguments.of("3a924d98-93d6-4c58-b358-999999999999", "9400-b382-3a924dc2-9999999999c3-4c82"));
  }
}

By default jqwik checks the property with 1000 new randomly generated UUIDs every time this test runs. This allows us to gradually explore the set of possible examples in order to find edge cases that we have not considered.

Discussion.

If we examine the resulting tests we may think that the property-based tests have made the example-based tests redundant. Should we delete the example-based tests and keep only the property-based ones?

Before answering this question, let’s think about each type of test from different points of view.

Understandability.

Despite being parameterized, it’s relatively easy to see which inputs and expected outputs are used by the example-based tests because they are literal values provided by the generateCipheringUuidExamples method. Besides, this kind of testing was more familiar to the team members.

In contrast, the UUID used by the property-based tests to check the property is randomly generated and the team was not familiar with property-based testing.

Granularity.

Since we are using a property that uses the “There and back again” pattern, if there were an error, we wouldn’t know whether the problem was in the encryption or the decryption function, not even after the shrinking process^[4]. We’d only know the initial UUID that made the property fail.

This might not be so when using other property patterns. For instance, when using a property based on the “The test oracle” pattern, we’d know the input and the actual and expected outputs in case of an error.

In contrast, using example-based testing it would be very easy to identify the location of the problem.

Confidence, thoroughness and exploration.

The example-based tests specify behaviour using concrete examples in which we set up concrete scenarios, and then check whether the effects produced by the behaviour match what we expect. In the case of the cipher, we pass an input to the functions and assert that their output is what we expect. The testing is reduced just to the arbitrary examples we were able to come up with, but there’s a “gap between what we claim to be testing and what we’re actually testing”^[5]: why those arbitrary examples? Does the cipher behave correctly for any possible example?

Property-based testing “approach the question of correctness from a different angle: under what preconditions and constraints (for example, the range of input parameters) should the functionality under test lead to particular postconditions (results of a computation), and which invariants should never be violated in the course?”^[6]. With property-based testing we are not limited to the arbitrary examples we were able to come up with as in example-based testing. Instead, property-based testing gives us thoroughness and the ability to explore because it’ll try to find examples that falsify a property every time the test runs. I think this ability to explore makes them more dynamic.

Implementation independence.

The example-based tests depend on the implementation of the cypher algorithm, whereas the property-based tests can be used for any implementation of the cypher algorithm because the decrypt_is_the_inverse_of_encrypt property is an invariant of any cipher algorithm implementation. This makes the property-based tests ideal to write a role test^[7] that any valid cipher implementation should pass.

Explicitness of invariants.

In the case of the cipher there’s a relationship between the encryption and decryption functions: they are inverses of each other.

This relationship might go completely untested using example-based testing if we use unrelated examples to test each of the functions. This means there could be changes to any of the functions that may violate the property while passing the independent separated tests of each function.

In the parameterized example-based tests we wrote, we implicitly tested this property by using the same set of examples for both functions just changing the roles of input and expected output for each test, but this is limited to the set of examples.

With property-based testing we are explicitly testing the relation between the two functions and exploring the space of inputs to try to find one that falsifies the property of being inverses of each other.

Protection against regressions.

Notice that, in this case, If we deleted the example-based tests and just kept the property-based test using the decrypt_is_the_inverse_of_encrypt property, we could introduce a simple regression by implementing both functions, encrypt and decrypt, as the identity function. That obviously wrong implementation would still fulfil the decrypt_is_the_inverse_of_encrypt property, which means that the property-based test using decrypt_is_the_inverse_of_encrypt property is not enough on its own to characterise the desired behaviour and protect it against regressions. We also need to at least add example-based testing for one of the cipher functions, either encrypt or decrypt. Notice that this might happen for any property based on “There and back again” pattern. This might not hold true for different contexts and property patterns.

What we did.

Given the previous discussion, we decided to keep both example-based and property-based tests in order to gain exploratory power while keeping familiarity, granularity and protection against regressions.

Summary.

We’ve shown a simple example of how we applied JUnit 5 parameterized tests to test the encryption and decryption functions of a cipher algorithm for UUIDs.

Then we showed a simple example of how we can use property-based testing to explore our solution and find edge cases. We also talked about how discovering properties can be the most difficult part of property-based testing, and how there are patterns that can be used to help us to discover them.

Finally, we discussed the resulting example-based and property-based tests from different points of view.

We hope this post will motivate you to start exploring property-based testing as well. If you want to learn more, follow the references we provide and start playing. Also have a look at the other posts exploring property-based testing in our blog we have written in the past.

Acknowledgements.

I’d like to thank my Codesai colleagues for reading the initial drafts and giving me feedback.

Notes.

[1] The experience of writing parameterized tests using JUnit 5 is so much better than it used to be with JUnit 4!

[2] Have a look at this other post in which I describe how property-based tests were able to find edge cases that I had not contemplated in a code with 100% test coverage that had been written using TDD.

[3] Scott Wlaschin’s article, Choosing properties for property-based testing, is a great post in which he manages to explain the patterns that have helped him the most, to discover the properties that are applicable to a given problem. Besides the “There and back again” pattern, I’ve applied the “The test oracle” on several occasions. Some time ago, I wrote a post explaining how I used it to apply property-based testing to an implementation of a binary search tree.
Another interesting article about the same topic is Property-based Testing Patterns, Sanjiv Sahayam.

[4] “Shrinking is the mechanism by which a property-based testing framework can be told how to simplify failure cases enough to let it figure out exactly what the minimal reproducible case is.” from chapter 8 of Fred Hebert’s PropEr Testing online book

[5] From David MacIver’s In praise of property-based testing post. According to David MacIver “the problem with example-based tests is that they end up making far stronger claims than they are actually able to demonstrate”.

[6] From Johannes Link’s Know for Sure with Property-Based Testing
post.

[7] Have a look at our recent post about role tests.

References.

Photo from Pixabay in Pexels.

Listening to test smells

Manuel Rivero — Mon, 02 Jan 2023 16:21:45 +0000

Introduction.

We'd like to show another example of how difficulties found while testing can signal design problems in our code^[1].

We believe that a good design is one that supports the evolution of a code base at a sustainable pace and that testability is a requirement for evolvability. This is not something new, we can find this idea in many places.

Michael Feathers says “every time you encounter a testability problem, there is an underlying design problem”^[2].

Nat Pryce and Steve Freeman also think that this relationship between testability and good design exists^[3]:

“[...] We’ve found that the qualities that make an object easy to test also make our code responsive to change”

and also use it to detect design problems and know what to refactor:

“[...] where we find that our tests are awkward to write, it's usually because the design of our target code can be improved”

and to improve their TDD practice:

“[...] sensitise yourself to find the rough edges in your tests and use them for rapid feedback about what to do with the code. […] don't stop at the immediate problem (an ugly test) but look deeper for why I'm in this situation (weakness in the design) and address that.”^[4]

This is why they devoted talks, several posts and a chapter of their GOOS book (chapter 20) to listening to the tests^[5] and even added it to the TDD cycle steps:

TDD cycle including listening to the tests.

Next we’ll show you an example of how we've recently applied this in a client.

The problem.

Recently I was asked to help a pair that was developing a new feature in a legacy code base of one of our clients. They were having problems with the following test^[6]:

package com.ads;

import com.ads.helpers.AdBuilder;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import java.util.Arrays;
import java.util.List;

import static com.ads.helpers.AdBuilder.aPromotionAd;
import static com.ads.helpers.SearchBuilder.aSearch;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.*;
import static org.mockito.Mockito.*;

class RealTimeGalleryAdsRepositoryTest {
  private static final String PHOTO = "imagen";
  private static final String TITLE = "titulo";
  private static final String URL = "url";
  private static final String DESCRIPTION = "descripcion";
  private static final long PRICE = 1L;
  private static final int BATHROOMS = 1;
  private static final int ROOMS = 1;
  private static final float BUILT_AREA = 1f;
  private static final String PROMOTION_TYPE_ID = "5";
  private static final String NO_LOCATION_ID = "0";
  private static final String COUNTRY = "co";
  private AdsRepository adsRepository;
  private RealTimeGalleryAdsRepository realTimeGalleryAdsRepository;
  private Search search;
  private Clock clock;
  private Configuration configuration;

  @BeforeEach
  public void setUp() {
    configuration = mock(Configuration.class);
    when(configuration.getPromotionPropertyType()).thenReturn(PROMOTION_TYPE_ID);

    clock = mock(Clock.class);
    when(configuration.getCountryId()).thenReturn(COUNTRY);

    search = aSearch()
        .withCountryId(COUNTRY)
        .withPropertyTypeId(PROMOTION_TYPE_ID)
        .withLocationId(NO_LOCATION_ID)
        .asSale()
        .build();

    adsRepository = mock(AdsRepository.class);

    boolean useCache = false;
    realTimeGalleryAdsRepository = createRealTimeGalleryAdsRepository(useCache);
  }

  @Test
  void maps_all_ads_with_photo_to_gallery_ads() {
    when(adsRepository.search(COUNTRY, search)).thenReturn(searchResult(
        ad("id1", "photo1").build(),
        ad("id2", "photo2").build(),
        ad("id3", "photo3").build()
    ));

    List<GalleryAd> galleryAds = realTimeGalleryAdsRepository.getAll();

    assertThat(galleryAds, contains(
        galleryAd("id1", "photo1"),
        galleryAd("id2", "photo2"),
        galleryAd("id3", "photo3")));
  }

  @Test
  public void ignore_ads_with_no_photos_in_gallery_ads() {
    when(adsRepository.search(COUNTRY, search)).thenReturn(
        searchResult(ad("id1").withNoPhoto().build())
    );

    List<GalleryAd> galleryAds = realTimeGalleryAdsRepository.getAll();

    assertThat(galleryAds, empty());
  }

  @Test
  void when_no_ads_are_found_there_are_no_gallery_ads() {
    when(adsRepository.search(COUNTRY, search)).thenReturn(noSearchResult());

    List<GalleryAd> galleryAds = realTimeGalleryAdsRepository.getAll();

    assertThat(galleryAds, empty());
  }

  @Test
  void when_cache_has_not_expired_the_cached_values_are_used() {
    realTimeGalleryAdsRepository = createRealTimeGalleryAdsRepository(true);
    realTimeGalleryAdsRepository.resetCache();
    when(adsRepository.search(COUNTRY, search)).thenReturn(searchResult(ad("id1").build()));

    List<GalleryAd> galleryAdsFirstResult = realTimeGalleryAdsRepository.getAll();
    List<GalleryAd> galleryAdsSecondResult = realTimeGalleryAdsRepository.getAll();

    assertThat(galleryAdsFirstResult, is(galleryAdsSecondResult));
    verify(adsRepository, times(1)).search(COUNTRY, search);
  }

  @Test
  void when_cache_expires_new_values_are_retrieved() {
    long maxCachedTime = 30 * 60 * 1000L;
    realTimeGalleryAdsRepository = createRealTimeGalleryAdsRepository(true);
    realTimeGalleryAdsRepository.resetCache();
    when(adsRepository.search(COUNTRY, search)).thenReturn(
        searchResult(ad("id1").build()));
    when(clock.getTimeInMillis()).thenReturn(0L, maxCachedTime + 1);

    realTimeGalleryAdsRepository.getAll();
    realTimeGalleryAdsRepository.getAll();

    verify(adsRepository, times(2)).search(COUNTRY, search);
  }

  private GalleryAd galleryAd(String id, String photo) {
    return new GalleryAd(
        id, photo, TITLE, URL, PRICE, DESCRIPTION, ROOMS,
        BATHROOMS, BUILT_AREA
    );
  }

  private SearchResult searchResult(Ad... ads) {
    return new SearchResult(Arrays.asList(ads));
  }

  private AdBuilder ad(String id) {
    return ad(id, PHOTO);
  }

  private AdBuilder ad(String id, String photo) {
    return aPromotionAd().
        withId(id).
        withDescription(DESCRIPTION).
        withTitle(TITLE)
        .withPrice(PRICE)
        .withNumberOfBathrooms(BATHROOMS)
        .withNumberOfRooms(ROOMS)
        .withBuiltArea(BUILT_AREA)
        .withUrl(URL)
        .withPhoto(photo);
  }

  private RealTimeGalleryAdsRepository createRealTimeGalleryAdsRepository(boolean useCache) {
    return new RealTimeGalleryAdsRepository(
        adsRepository, configuration,
        useCache, clock);
  }

  private SearchResult noSearchResult() {
    return null;
  }
}

that was testing the RealTimeGalleryAdsRepository class:

package com.ads;

import java.util.Collections;
import java.util.List;
import java.util.stream.Collectors;

public class RealTimeGalleryAdsRepository implements GalleryAdsRepository {
  private final boolean useCache;
  private final Clock clock;
  private static SearchResult cachedSearchResult;
  private static long cachedTime;
  private final Configuration configuration;
  private final AdsRepository adsRepository;

  public RealTimeGalleryAdsRepository(
      AdsRepository adsRepository,
      Configuration configuration,
      boolean useCache, Clock clock) {

    this.adsRepository = adsRepository;
    this.configuration = configuration;
    this.useCache = useCache;
    this.clock = clock;
  }

  @Override
  public List<GalleryAd> getAll() {
    SearchResult searchResult = getCachedSearchResult();
    if (searchResult == null) {
      return Collections.emptyList();
    }
    return searchResult.getAds().stream()
        .filter(ad -> ad.hasPhoto())
        .map(GalleryAd::fromAd)
        .collect(Collectors.toList());
  }

  private SearchResult getCachedSearchResult() {
    if (!useCache) {
      return getSearchResult();
    }
    long timeInMillis = clock.getTimeInMillis();
    if (cacheIsExpired(timeInMillis)) {
      cachedSearchResult = null;
    }
    if (cachedSearchResult == null) {
      cachedSearchResult = getSearchResult();
      cachedTime = timeInMillis;
    }
    return cachedSearchResult;
  }

  private boolean cacheIsExpired(long timeInMillis) {
    return cachedTime + (30 * 60 * 1000L) < timeInMillis;
  }

  private SearchResult getSearchResult() {
    return adsRepository.search(configuration.getCountryId(), createSearch());
  }

  private Search createSearch() {
    String projectPropertyTypeId = configuration.getPromotionPropertyType();
    String noLocationId = "0";
    return new Search(
        configuration.getCountryId(),
        OperationTypes.Sale,
        projectPropertyTypeId,
        noLocationId);
  }

  public void resetCache() {
    cachedSearchResult = null;
  }
}

They had managed to test drive the functionality but they were unhappy with the results. The thing that was bothering them was the resetCache method in the RealTimeGalleryAdsRepository class. As its name implies, its intent was to reset the cache. This would have been fine, if this had been a requirement, but that was not the case. The method had been added only for testing purposes.

Looking at the code of RealTimeGalleryAdsRepository you can learn why.
The cachedSearchResult field is static and that was breaking the isolation between tests.
Even though they were using different instances of RealTimeGalleryAdsRepository in each test, they were sharing the same value of the cachedSearchResult field because static state is associated with the class. So a new public method, resetCache, was added to the class only to ensure isolation between different tests.

Adding code to your production code base just to enable unit testing is a unit testing anti-pattern^[7], but they didn’t know how to get rid of the resetCache method, and that’s why I was called in to help.

Let’s examine the tests in RealTimeGalleryAdsRepositoryTests to see if they can point to more fundamental design problems.

Another thing we can notice is that the tests can be divided in to sets that are testing two very different behaviours:

One set of tests, comprised of maps_all_ads_with_photo_to_gallery_ads, ignore_ads_with_no_photos_in_gallery_ads and when_no_ads_are_found_there_are_no_gallery_ads, is testing the code that obtains the list of gallery ads;
whereas, the other set, comprised of when_cache_has_not_expired_the_cached_values_are_used and when_cache_expires_new_values_are_retrieved is testing the life and expiration of some cached values.

This lack of focus was a hint that the production class might lack cohesion, i.e., it might have several responsibilities.

It turns out that there was another code smell that confirmed our suspicion. Notice the boolean parameter useCache in RealTimeGalleryAdsRepository constructor.
That was a clear example of a flag argument^[8]. useCache was making the class behave differently depending on its value:

It cached the list of gallery ads when useCache was true.
It did not cache them when useCache was false.

After seeing all this, I told the pair that the real problem was the lack of cohesion and that we’d have to go more object-oriented in order to avoid it. After that refactoring the need for the resetCache would disappear.

Going more OO to fix the lack of cohesion.

To strengthen cohesion we need to separate concerns. Let’s see the problem from the point of view of the client of the RealTimeGalleryAdsRepository class, (this point of view is generally very useful because the test is also a client of the tested class) and think about what it would want from the RealTimeGalleryAdsRepository. It would be something like “obtain the gallery ads for me”, that would be the responsibility of the RealTimeGalleryAdsRepository, and that’s what the GalleryAdsRepository represents.

Notice that to satisfy that responsibility we do not need to use a cache, only get some ads from the AdsRepository and map them (the original functionality also included some enrichments using data from other sources but we remove them from the example for the sake of simplicity). Caching is an optimization that we might do or not, it’s a refinement or embellishment to how we satisfy the responsibility but it’s not necessary to satisfy it. In this case, caching changes the “how” but not the “what”.

This matches very well with the Decorator design pattern because this pattern “comes into play when there are a variety of optional functions that can precede or follow another function that is always executed”^[9]. Using it would allow us to attach additional behaviour (caching) to the basic required behaviour that satisfied the role that the client needs (“obtain the gallery ads for me”). This way instead of having a flag parameter (like useCache in the original code) to control whether we cache or not, we might add caching by composing objects that implement the GalleryAdsRepository. One of them, RealTimeGalleryAdsRepository, would be in charge of getting ads from the AdsRepository and mapping them to gallery ads; and the other one, CachedGalleryAdsRepository, would cache the gallery ads.

So we moved the responsibility of caching the ads to the CachedGalleryAdsRepository class which decorated the RealTimeGalleryAdsRepository class.

This is the code of the CachedGalleryAdsRepository class:

package com.ads.domain;

import java.time.Duration;
import java.util.List;

public class CachedGalleryAdsRepository implements GalleryAdsRepository {
  private final GalleryAdsRepository galleryAdsRepository;
  private final Clock clock;
  private final Duration duration;
  private List<GalleryAd> cachedGalleryAds;
  private long lastRefreshTime;

  public CachedGalleryAdsRepository(
      GalleryAdsRepository galleryAdsRepository,
      Clock clock,
      Duration duration) {
    this.galleryAdsRepository = galleryAdsRepository;
    this.clock = clock;
    this.duration = duration;
  }

  @Override
  public List<GalleryAd> getAll() {
    long timeInMillis = clock.getTimeInMillis();
    if (needToRefreshCache(timeInMillis)) {
      refreshCache(timeInMillis);
    }
    return cachedGalleryAds;
  }

  private boolean needToRefreshCache(long timeInMillis) {
    return cachedGalleryAds == null || hasCacheExpired(timeInMillis);
  }

  private boolean hasCacheExpired(long timeInMillis) {
    return duration.toMillis() + lastRefreshTime < timeInMillis;
  }

  private void refreshCache(long timeInMillis) {
    lastRefreshTime = timeInMillis;
    cachedGalleryAds = galleryAdsRepository.getAll();
  }
}

and these are its tests:

package com.ads;

import com.ads.domain.CachedGalleryAdsRepository;
import com.ads.domain.Clock;
import com.ads.domain.GalleryAd;
import com.ads.domain.GalleryAdsRepository;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import java.time.Duration;
import java.util.List;

import static com.ads.helpers.GalleryAdBuilder.aGalleryAd;
import static java.util.Collections.singletonList;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;
import static org.mockito.Mockito.*;

class CachedGalleryAdsRepositoryTest {
  private static final Duration CACHE_DURATION = Duration.ofMinutes(30);
  private Clock clock;
  private GalleryAdsRepository galleryAdsRepository;
  private CachedGalleryAdsRepository cachedGalleryAdsRepository;

  @BeforeEach
  void setUp() {
    clock = mock(Clock.class);
    galleryAdsRepository = mock(GalleryAdsRepository.class);

    cachedGalleryAdsRepository = new CachedGalleryAdsRepository(
        galleryAdsRepository,
        clock,
        CACHE_DURATION
    );
  }

  @Test
  void when_cache_has_not_expired_the_cached_values_are_used() {
    when(clock.getTimeInMillis()).thenReturn(0L, CACHE_DURATION.toMillis());
    when(galleryAdsRepository.getAll()).thenReturn(
        singletonList(aGalleryAd().withId("id1").build())
    );

    List<GalleryAd> adsGalleryFirstResult = cachedGalleryAdsRepository.getAll();
    List<GalleryAd> adsGallerySecondResult = cachedGalleryAdsRepository.getAll();

    verify(galleryAdsRepository, times(1)).getAll();
    assertThat(adsGalleryFirstResult, is(adsGallerySecondResult));
  }

  @Test
  void when_cache_expires_new_values_are_retrieved() {
    when(clock.getTimeInMillis()).thenReturn(
        CACHE_DURATION.toMillis(),
        CACHE_DURATION.toMillis() + 1,
        2 * CACHE_DURATION.toMillis() + 1);
    List<GalleryAd> someGalleryAds = singletonList(aGalleryAd().withId("id1").build());
    List<GalleryAd> anotherGalleryAds = singletonList(aGalleryAd().withId("id3").build());
    when(galleryAdsRepository.getAll()).thenReturn(someGalleryAds, anotherGalleryAds);

    List<GalleryAd> galleryAdsFirstResult = cachedGalleryAdsRepository.getAll();
    List<GalleryAd> galleryAdsSecondResult = cachedGalleryAdsRepository.getAll();
    List<GalleryAd> galleryAdsThirdResult = cachedGalleryAdsRepository.getAll();

    verify(galleryAdsRepository, times(2)).getAll();
    assertThat(galleryAdsFirstResult, is(someGalleryAds));
    assertThat(galleryAdsSecondResult, is(galleryAdsFirstResult));
    assertThat(galleryAdsThirdResult, is(anotherGalleryAds));
  }
}

Notice how we found here again the two tests that were previously testing the life and expiration of the cached values in the test of the original RealTimeGalleryAdsRepository: when_cache_has_not_expired_the_cached_values_are_used and when_cache_expires_new_values_are_retrieved.

Furthermore, looking at them more closely, we can see how, in this new design, those tests are also simpler because they don't know anything about the inner details
of RealTimeGalleryAdsRepository. They only know about the logic related to the life and expiration of the cached values and that when the cache is refreshed they call a collaborator that implements the GalleryAdsRepository interface, this means that now we're caching gallery ads instead of an instance of the SearchResult and we don't know anything about the AdsRepository.

On a side note, we also improved the code by using the Duration value object from java.time to remove the primitive obsession smell caused by using a long to represent milliseconds.

Another very important improvement is that we don’t need the static field anymore.

And what about RealTimeGalleryAdsRepository?

package com.ads.domain;

import java.util.List;
import java.util.stream.Collectors;

import static java.util.Collections.emptyList;

public class RealTimeGalleryAdsRepository implements GalleryAdsRepository {
  private final Configuration configuration;
  private final AdsRepository adsRepository;
  private static final String NO_LOCATION_ID = "0";

  public RealTimeGalleryAdsRepository(
      AdsRepository adsRepository,
      Configuration configuration) {
    this.adsRepository = adsRepository;
    this.configuration = configuration;
  }

  @Override
  public List<GalleryAd> getAll() {
    SearchResult searchResult = adsRepository.search(
        configuration.getCountryId(),
        createSearch(configuration.getPromotionPropertyType())
    );

    if (searchResult == null) {
      return emptyList();
    }

    return searchResult.getAds().stream()
        .filter(ad -> ad.hasPhoto())
        .map(GalleryAd::fromAd)
        .collect(Collectors.toList());
  }

  private Search createSearch(String projectPropertyTypeId) {
    return new Search(
        configuration.getCountryId(),
        OperationTypes.Sale,
        projectPropertyTypeId,
        NO_LOCATION_ID);
  }
}

If we have a look at its new code, we can notice that its only concern is how to obtain the list of gallery ads and mapping them from the result of its collaborator AdsRepository, and it does not know anything about caching values. So the new design is more cohesive than the original one.
Notice how we removed both the resetCache method that was before polluting its interface only for testing purposes, and the flag argument, useCache, in the constructor.

We also reduced its number of collaborators because there’s no need for a Clock anymore. That collaborator was needed for a different concern that is now taken care of in the decorator CachedGalleryAdsRepository.

These design improvements are reflected in its new tests. They are now more focused, and can only fail if the obtention of the gallery ads breaks. Having only one reason to fail comes from testing a more cohesive unit with only one reason to change. Notice how these tests coincide with the subset of tests concerned with testing the same behaviour in the original tests of RealTimeGalleryAdsRepository:

package com.ads;

import com.ads.domain.*;
import com.ads.helpers.AdBuilder;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import java.util.Arrays;
import java.util.List;

import static com.ads.helpers.AdBuilder.aPromotionAd;
import static com.ads.helpers.SearchBuilder.aSearch;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.empty;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

class RealTimeGalleryAdsRepositoryTest {
  private static final String TITLE = "title";
  private static final String URL = "url";
  private static final String DESCRIPTION = "description";
  private static final long PRICE = 1L;
  private static final int BATHROOMS = 1;
  private static final int ROOMS = 1;
  private static final float BUILT_AREA = 1f;
  private static final String PROMOTION_TYPE_ID = "5";
  private static final String NO_LOCATION_ID = "0";
  private static final String COUNTRY = "co";
  private AdsRepository adsRepository;
  private RealTimeGalleryAdsRepository realTimeGalleryAdsRepository;
  private Search search;
  private Configuration configuration;

  @BeforeEach
  public void setUp() {
    configuration = mock(Configuration.class);
    when(configuration.getPromotionPropertyType()).thenReturn(PROMOTION_TYPE_ID);
    when(configuration.getCountryId()).thenReturn(COUNTRY);

    search = aSearch()
        .withCountryId(COUNTRY)
        .withPropertyTypeId(PROMOTION_TYPE_ID)
        .withLocationId(NO_LOCATION_ID)
        .asSale()
        .build();

    adsRepository = mock(AdsRepository.class);

    realTimeGalleryAdsRepository = new RealTimeGalleryAdsRepository(
        adsRepository, configuration
    );
  }

  @Test
  void maps_all_ads_with_photo_to_gallery_ads() {
    when(adsRepository.search(COUNTRY, search)).thenReturn(
        searchResult(
            ad("id1", "photo1.jpg").build(),
            ad("id2", "photo2.jpg").build(),
            ad("id3", "photo3.jpg").build()));

    List<GalleryAd> galleryAds = realTimeGalleryAdsRepository.getAll();

    assertThat(galleryAds, contains(
        galleryAd("id1", "photo1.jpg"),
        galleryAd("id2", "photo2.jpg"),
        galleryAd("id3", "photo3.jpg"))
    );
  }

  @Test
  void when_no_ads_are_found_there_are_no_gallery_ads() {
    when(adsRepository.search(COUNTRY, search)).thenReturn(noSearchResult());

    List<GalleryAd> galleryAds = realTimeGalleryAdsRepository.getAll();

    assertThat(galleryAds, empty());
  }

  @Test
  public void ignore_ads_with_no_photos_in_gallery_ads() {
    when(adsRepository.search(COUNTRY, search)).thenReturn(
        searchResult(aPromotionAd().withId("id1").withNoPhoto().build()));

    List<GalleryAd> galleryAds = realTimeGalleryAdsRepository.getAll();

    assertThat(galleryAds, empty());
  }

  private GalleryAd galleryAd(String id, String photo) {
    return new GalleryAd(
        id, photo, TITLE, URL, PRICE, DESCRIPTION, ROOMS,
        BATHROOMS, BUILT_AREA
    );
  }

  private SearchResult searchResult(Ad... ads) {
    return new SearchResult(Arrays.asList(ads));
  }

  private AdBuilder ad(String id, String photo) {
    return aPromotionAd().
        withId(id).
        withDescription(DESCRIPTION).
        withTitle(TITLE)
        .withPrice(PRICE)
        .withNumberOfBathrooms(BATHROOMS)
        .withNumberOfRooms(ROOMS)
        .withBuiltArea(BUILT_AREA)
        .withUrl(URL)
        .withPhoto(photo);
  }

  private SearchResult noSearchResult() {
    return null;
  }
}

Persisting the cached values between calls.

You might be asking yourselves, how are we going to ensure that the cached values persist between calls now that we don't have a static field anymore.

Well, the answer is that we don't need to keep a static field in our classes for that. The only thing we need is that the composition of CachedGalleryAdsRepository and RealTimeGalleryAdsRepository is created only once, and that we use that single instance for the lifetime of the application. That is a concern that we can address using a different mechanism.

We usually find in legacy code bases that this need to create something only once, and use that single instance for the lifetime of the application is met using the Singleton design pattern described in the design patterns book. The Singleton design pattern intent is to “ensure that only one instance of the singleton class ever exists; and provide global access to that instance”. The second part of that intent, “providing global access”, is problematic because it introduces global state into the application. Using global state creates high coupling (in the form of hidden dependencies and possible actions at a distance) that drastically reduces testability.

Instead we used the singleton pattern^[10]. Notice the lowercase letter. The lowercase ’s’ singleton avoids those testability problems because its intent is only to “ensure that only one instance of some class ever exists because its new operator is called only once”. The problematic global access part gets removed from the intent. This is done by avoiding mixing object instantiation with business logic by using separated factories that know how to create and wire up all the dependencies using dependency injection.

We might create this singleton, for instance, by using a dependency injection framework like Guice and its @Singleton annotation.

In this case we coded it ourselves:

package com.ads.factories;

import com.ads.domain.CachedGalleryAdsRepository;
import com.ads.domain.GalleryAdsRepository;
import com.ads.domain.RealTimeGalleryAdsRepository;
import com.ads.infrastructure.ConfigurationsByCountry;
import com.ads.infrastructure.RealTimeAdsRepository;
import com.ads.infrastructure.SystemClock;

import java.time.Duration;
import java.util.HashMap;
import java.util.Map;

public class GalleryAdsRepositoryFactory {
  private static final Duration TIME_TO_EXPIRE_CACHE = Duration.ofMinutes(30L);
  private static Map<Integer, GalleryAdsRepository> instanceByCountry = new HashMap<>();

  public static GalleryAdsRepository createUniqueCached(int countryId) {
    if (!instanceByCountry.containsKey(countryId)) {
      instanceByCountry.put(countryId, createInstance(countryId));
    }
    return instanceByCountry.get(countryId);
  }

  private static GalleryAdsRepository createInstance(int countryId) {
    GalleryAdsRepository galleryAdsRepository = new RealTimeGalleryAdsRepository(
        new RealTimeAdsRepository(countryId),
        ConfigurationsByCountry.get(countryId)
    );
    return new CachedGalleryAdsRepository(
        galleryAdsRepository,
        new SystemClock(),
        TIME_TO_EXPIRE_CACHE);
  }
}

Notice the factory method that returns a unique instance of the GalleryAdsRepository interface that caches values. This factory method is never used by business logic, it’s only used by instantiation logic in factories that know how to create and wire up all the dependencies using dependency injection. This doesn’t introduce testability problems because the unique instance will be injected through constructors by factories wherever is needed.

Conclusions.

We show a recent example we found working for a client that illustrates how testability problems may usually point, if we listen to them, to the detection of underlying design problems. In this case the problems in the test were pointing to a lack of cohesion in the production code that was being tested. The original class had too many responsibilities.

We refactored the production code to separate concerns by going more OO applying the decorator design pattern. The result was more cohesive production classes that led to more focused tests, and removed the design problems we had detected in the original design.

Acknowledgements.

I’d like to thank my Codesai colleagues for reading the initial drafts and giving me feedback.

Notes.

[1] We showed another example of this relationship between poor testability and design problems in a previous post: An example of listening to the tests to improve a design.

[2] Listen to his great talk about this relationship: The Deep Synergy Between Testability and Good Design

[3] This is the complete paragraph from chapter 20, Listening to the tests, of the GOOS book: “Sometimes we find it difficult to write a test for some functionality we want to add to our code. In our experience, this usually means that our design can be improved — perhaps the class is too tightly coupled to its environment or does not have clear responsibilities. When this happens, we first check whether it’s an opportunity to improve our code, before working around the design by making the test more complicated or using more sophisticated tools. We’ve found that the qualities that make an object easy to test also make our code responsive to change.”

[4] This quote is from their post Synaesthesia: Listening to Test Smells.

[5] Have a look at this interesting series of posts about listening to the tests by Steve Freeman. It’s a raw version of the content that you’ll find in chapter 20, Listening to the tests, of their book.

[6] We have simplified the client's code to remove some distracting details and try to highlight its key problems.

[7] Vladimir Khorikov calls this unit testing anti-pattern Code pollution.

[8] A flag Argument is a kind of argument that is telling a function or a class to behave in a different way depending on its value. This might be a signal of poor cohesion in the function or class.

[9] Have a look at the discussion in the chapter devoted to the Decorator design pattern in the great book Design Patterns Explained: A New Perspective on Object-Oriented Design.

[10] Miško Hevery talks about the singleton pattern with lowercase ‘s’ in its talk Global State and Singletons
at 10:20: “Singleton with capital ’S’. Refers to the design pattern where the Singleton has a private constructor and has a global instance variable. Lowercase ’s’ singleton means I only have a single instance of something because I only called the new operator once.”

References.

Photo from cottonbro in Pexels.

Role tests for interfaces discovered through TDD

Manuel Rivero — Tue, 27 Dec 2022 18:54:37 +0000

Introduction.

Working through the first three iterations of a workshop's exercise, we produced several application services that at some point collaborated with a users repository that we hadn't yet created so we used a test double in its place in their tests.

These are the tests:

require 'spec_helper'
require 'cos/actions/users/register_user'
require 'cos/core/users/errors'

describe Actions::RegisterUser do
  let(:users_repository) { double('UsersRepository') }
  let(:new_user_name) {'@foolano'}

  before do
    stub_const('Users::Repository', users_repository)
  end

  describe "Registering a user" do
    it "can register a user that is not already registered" do
      allow(users_repository).
        to receive(:registered?).
        with(new_user_name).and_return(false)
      expect(users_repository).
        to receive(:register).with(new_user_name)

      Actions::RegisterUser.do(new_user_name)
    end

    it "fails when trying to register a user that is already registered" do
      allow(users_repository).
        to receive(:registered?).with(new_user_name).and_return(true)

      expect{Actions::RegisterUser.do(new_user_name)}.
        to raise_error(Users::Errors::AlreadyRegistered)
    end
  end
end

require 'spec_helper'
require 'cos/actions/users/follow_user'
require 'cos/core/users/errors'

describe Actions::FollowUser do

  let(:follower_name) { "foolano" }
  let(:followed_name) { "mengano" }
  let(:users_repository) { double('UsersRepository') }

  before do
    stub_const('Users::Repository', users_repository)
  end

  describe "Following a user" do
    describe "when both users are registered" do
      it "succesfully adds a follower to a followed user" do
        allow(users_repository).
          to receive(:registered?).with(follower_name).and_return(true)
        allow(users_repository).
          to receive(:registered?).with(followed_name).and_return(true)
        expect(users_repository).
          to receive(:add_follower).with(follower_name, followed_name)

        Actions::FollowUser.do follower_name, followed_name
      end
    end

    describe "when any of them is not registered" do
      it "raises an error when trying to add a registered follower to a followed user that does not exist" do
        allow(users_repository).
          to receive(:registered?).with(follower_name).and_return(true)
        allow(users_repository).
          to receive(:registered?).with(followed_name).and_return(false)

        expect {Actions::FollowUser.do follower_name, followed_name}.
          to raise_error(Users::Errors::NonRegistered)
      end

      it "raises an error when trying to add a follower that does not exist to a registered followed user" do
        allow(users_repository).
          to receive(:registered?).with(follower_name).and_return(false)
        allow(users_repository).
          to receive(:registered?).with(followed_name).and_return(true)

        expect{Actions::FollowUser.do follower_name, followed_name}.
          to raise_error(Users::Errors::NonRegistered)
      end
    end
  end
end

require 'spec_helper'
require 'cos/queries/users/followers_of_user'

describe Queries::FollowersOfUser do
  let(:follower_names) {["pepe", "juan"]}
  let(:followed_name) {"koko"}
  let(:users_repository) { double('UsersRepository') }

  before do
    stub_const('Users::Repository', users_repository)
  end

  describe "Getting the followers of a user" do
    it "returns the list of followers" do
      allow(users_repository).to receive(:followers_of)
      .with(followed_name)
      .and_return(follower_names)

      expect(Queries::FollowersOfUser.do(followed_name)).to eq follower_names
    end
  end
end

In these tests, every time we allow or expect a method call on our repository double,
we are defining not only the messages that the users repository can respond to (its public interface)^[1] but also what its clients can expect from each of those messages, i.e. its contract.

In other words, at the same time we were testing the application services, we defined from the point of view of its clients the responsibilities that the users repository should be accountable for.

The users repository is at the boundary of our domain. It's a port that allows us to not have to know anything about how users are stored, found, etc. This way we are able to just focus on what its clients want it to do for them, i.e., its responsibilities.

Focusing on the responsibilities results in more stable interfaces. As I heard Sandi Metz say once:

"You can trade the unpredictability of what others do for the constancy of what you want."^[2]

which is a very nice way to explain the "Program to an interface, not an implementation"^[3] design principle.

How those responsibilities are carried out is something that each different implementation (or adapter) of the users repository port is responsible for. However, the terms of the contract that its clients rely on, must be respected by all of the adapters. They must play their roles. In this sense, any adapter must be substitutable by any other without the clients being affected, (yes, you're right, it's the Liskov substitution principle).

Role or contract tests.

The only way to ensure this substitutability is by testing each adapter to check if it also respects the terms of the contract, i. e. it fulfils its role. Those tests would ensure that the Liskov substitution principle is respected^[4].

I will use the term role test used by Sandi Metz because contract test has become overloaded^[5].

Ok, but how can we test that all the possible implementations of the user repository respect the contract without repeating a bunch of test code?

Using shared examples in RSpec to write role tests.

There’s one very readable way to do it in Ruby using RSpec.

We created a RSpec shared example in a file named users_repository_role.rb where we wrote the tests that describes the behaviour that users repository clients were relying on:

require 'spec_helper'

RSpec.shared_examples "users repository" do
  let(:user_name) { '@foolano' }
  let(:follower_name) { '@zutano' }
  let(:followed_name) { '@mengano' }

  it "knows when a user is registered" do
    given_already_registered(user_name)

    expect(@repo.registered?(user_name)).to be_truthy
  end

  it "knows when a user is not registered" do
    expect(@repo.registered?(user_name)).to be_falsy
  end

  it "registers a new user" do
    @repo.register(user_name)

    expect(@repo.registered?(user_name)).to be_truthy
  end

  it "adds a follower to a user" do
    given_both_already_registered(follower_name, followed_name)

    @repo.add_follower(follower_name, followed_name)

    expect(@repo.followers_of(followed_name)).to eq [follower_name]
  end

  def given_both_already_registered(follower_name, followed_name)
    given_already_registered(follower_name)
    given_already_registered(followed_name)
  end

  def given_already_registered(user_name)
    @repo.register(user_name)
  end
end

Then for each implementation of the users repository you just need to include the role tests using RSpec it_behaves_like method, as shown in the following two implementations:

require 'spec_helper'
require 'infrastructure/users/repositories/in_memory'
require_relative '../cos/repositories_contracts/users_repository_role'

describe "In memory users repository" do
  before do
    @repo = Users::Repositories::InMemory.new
  end

  it_behaves_like "users repository"
end

require 'spec_helper'
require 'infrastructure/users/repositories/mongoid'
require_relative '../cos/repositories_contracts/users_repository_role'

describe "Mongoid users repository" do
  before do
    @repo = Users::Repositories::Mongoid.new
  end

  it_behaves_like "users repository"
end

You could still add any other test that only has to do with a given implementation in its specific test.

This solution is very readable and reduces a lot of duplication in the tests. However, the idea of role tests is not only important from the point of view of avoiding duplication in test code. In dynamic languages, such as Ruby, they also serve as a mean to highlight and document the role of duck types that might otherwise go unnoticed because there is no interface construct.

Acknowledgements.

I’d like to thank my Codesai colleagues for reading the initial drafts and giving me feedback.

Notes.

[1] Read more about objects communicating by sending and receiving messages in Alan Kay's Definition Of Object Oriented

[2] You can find a slightly different wording of it in her great talk Less - The Path to Better Design at 29’48’’.

[3] Presented in chapter one of Design Patterns: Elements of Reusable Object-Oriented Software book.

[4] This is similar to J. B. Rainsberger's idea of contract tests mentioned in his Integrated Tests Are A Scam talk and also to Jason Gorman's idea of polymorphic testing.

[5] For example Martin Fowler uses contract test to define a different concept in Contract Test.

References.

Photo from Anna Rye in Pexels

Solving the Beverages Prices Refactoring kata (2)

Manuel Rivero — Sat, 24 Dec 2022 17:00:39 +0000

Introduction.

This is the second and last post in a series of posts showing a possible solution to the Beverages Prices Refactoring kata that I recently developed with some people from Women Tech Makers Barcelona with whom I'm working through Codesai's Practice Program twice a month.

In the previous post we introduced a design based on composition that fixed the Combinatorial Explosion code smell and produced a flexible solution applying the decorator design pattern. There was a potential problem in that solution because the client code, the code that needed to find out the price of the beverages, knew^[1] too much about how to create and compose beverages and supplements.

Have a look, for instance, at the following line new WithCream(new WithMilk(new Coffee())). It knows about three classes and how they are being composed. In the case of this kata, that might not be a big problem, since the client code is only comprised of a few tests, but, in a larger code base, this problem might spread across numerous classes generating a code smell known as Creation Sprawl^[2]
In this post, we'll try to reduce client knowledge of concrete component and decorator classes and their composition by encapsulating all the creational knowledge behind a nice, readable interface that we'll keep all the complexity of combining the supplements (decorators) and beverages (components) hidden from the client code.

Another more subtle problem with this design based on composition has to do with something that we have lost: the fact that not all combinations of beverages and supplements were allowed on the menu. That knowledge was implicitly encoded in the initial inheritance hierarchy, and disappeared with it. In the current design we can dynamically create any combination of beverages and supplements, including those that were not included in the original menu, like, for instance a tea with cinnamon, milk and cream (doing new WithCinnamon(new WithCream(new WithMilk(new Tea())))) which you might find delicious :).We'll also explore possible ways to recover that limitation of options.

We'll start by examining some creational patterns that are usually applied along with the decorator design pattern.

Would the Factory pattern help?

In order to encapsulate the creational code and hide its details from client code, we might use the factory pattern described by Joshua Kerievsky in his Refactoring to Patterns. A factory is a class that implements one or more Creation Methods. A Creation Method is a static or non-static method the creates and returns an object instance^[3].

We might apply the Encapsulate Classes with Factory refactoring^[4] to introduce a factory class with an interface which provided a creation method for each entry on the menu, that is, it would have a method for making coffee, another one for making tea, another one for making coffee with milk, and so on, and so forth.

Before starting to refactor, let’s think a bit about the consequences of introducing this pattern to assess if it will leave us in a better design spot or not. At first sight, introducing the factory pattern seems to simplify client code and reduce the overall coupling because it encapsulates all the creational logic hiding the complexity related to composing decorators and components behind its interface which solves the first problem we discussed in the introduction. The second one, limiting the combinations of beverages and supplements to only the ones available on the menu, is solved just by limiting the methods in the interface of the factory.

However, it would also create a maintenance problem somehow similar to the initial combinatorial explosion code smell we were trying to avoid when we decided to introduce the decorator design pattern. As we said, the interface of the factory would have a method for each combination of beverages and supplements available on the menu. This means that to add a new supplement we’d have to multiply the number of Creation methods in the interface of the factory by two. So, we might say that, introducing the factory pattern, we’d get an interface suffering from a combinatorial explosion of methods^[5].

Knowing that, we might conclude that a solution using the factory pattern would be interesting only when having a small number of options or if we didn’t expect the number of supplements to grow. As we said in the previous post, we think it likely that we’ll be required to add new supplements so we prefer a design that is easy to evolve along the axis of change of adding new supplements^[6]. This means the factory pattern is not the way to go for us this time because it’s not flexible enough for our current needs. We'll have to explore more flexible alternatives^[7].

Let’s try using the Builder design pattern.

The builder design pattern is often used for constructing complex and/or composite objects^[8]. Using it we might create a nice readable interface to compose the beverages and supplements bit by bit. Like the factory pattern, a builder would encapsulate the complexity of combining decorators from the client code. Unlike the factory pattern, a builder allows to construct the composite following a varying process. It’s this last characteristic that will avoid the combinatorial explosion of methods that made us discard the factory pattern.

In this case you can introduce the builder by applying the Encapsulate Composite with Builder. Let’s have a look at how we implemented it:

package coffee_shop;

import coffee_shop.menu.beverages.Coffee;
import coffee_shop.menu.beverages.HotChocolate;
import coffee_shop.menu.beverages.Tea;
import coffee_shop.menu.supplements.WithCinnamon;
import coffee_shop.menu.supplements.WithCream;
import coffee_shop.menu.supplements.WithMilk;

public class BeverageMachine {
  public static BeverageMachine coffee() {
    return new BeverageMachine(new Coffee());
  }

  public static BeverageMachine tea() {
    return new BeverageMachine(new Tea());
  }

  public static BeverageMachine hotChocolate() {
    return new BeverageMachine(new HotChocolate());
  }

  private Beverage beverage;

  private BeverageMachine(Beverage beverage) {
    this.beverage = beverage;
  }

  public Beverage make() {
    return beverage;
  }

  public BeverageMachine withMilk() {
    beverage = new WithMilk(beverage);
    return this;
  }

  public BeverageMachine withCinnamon() {
    beverage = new WithCinnamon(beverage);
    return this;
  }

  public BeverageMachine withCream() {
    beverage = new WithCream(beverage);
    return this;
  }
}

Notice how we keep the state of the partially composed object and apply the decorations incrementally until it’s returned by the make method. Notice also how the beverage is the initial state in the process of creating the composite object.

These are the tests after introducing the builder design pattern:

package unit_tests;

import coffee_shop.Beverage;
import coffee_shop.BeverageMachine;
import org.junit.Assert;
import org.junit.Test;

import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.closeTo;

public class BeveragesPricingTest {

  private static final double PRECISION = 0.001;

  @Test
  public void computes_coffee_price() {
    Beverage coffee = BeverageMachine.coffee().make();
    assertThat(coffee.price(), is(closeTo(1.20, PRECISION)));
  }

  @Test
  public void computes_tea_price() {
    Beverage tea = BeverageMachine.tea().make();
    assertThat(tea.price(), is(closeTo(1.50, 0.001)));
  }

  @Test
  public void computes_hot_chocolate_price() {
    Beverage hotChocolate = BeverageMachine.hotChocolate().make();
    assertThat(hotChocolate.price(), is(closeTo(1.45, 0.001)));
  }

  @Test
  public void computes_tea_with_milk_price() {
    Beverage teaWithMilk = BeverageMachine.tea().withMilk().make();
    assertThat(teaWithMilk.price(), is(closeTo(1.60, 0.001)));
  }

  @Test
  public void computes_tea_with_cinnamon_price() {
    Beverage teaWithCinnamon = BeverageMachine.tea().withCinnamon().make();
    Assert.assertThat(teaWithCinnamon.price(), is(closeTo(1.55, 0.001)));
  }

  @Test
  public void computes_tea_with_milk_and_cinnamon_price() {
    Beverage teaWithMilkAndCinnamon = BeverageMachine.tea().withMilk().withCinnamon().make();
    Assert.assertThat(teaWithMilkAndCinnamon.price(), is(closeTo(1.65,0.001)));
  }

  @Test
  public void computes_coffee_with_milk_price() {
    Beverage coffeWithMilk = BeverageMachine.coffee().withMilk().make();
    assertThat(coffeWithMilk.price(), is(closeTo(1.30, 0.001)));
  }

  @Test
  public void computes_coffee_with_milk_and_cream_price() {
    Beverage coffeeWithMilkAndCream = BeverageMachine.coffee().withMilk().withCream().make();
    assertThat(coffeeWithMilkAndCream.price(), is(closeTo(1.45, 0.001)));
  }

  @Test
  public void computes_hot_chocolate_with_cream_price() {
    Beverage hotChocolateWithCream = BeverageMachine.hotChocolate().withCream().make();
    assertThat(hotChocolateWithCream.price(),  is(closeTo(1.60, 0.001)));
  }

  @Test
  public void computes_coffee_with_cinnamon_price() {
    Beverage coffeeWithCinamon = BeverageMachine.coffee().withCinnamon().make();
    Assert.assertThat(coffeeWithCinamon.price(), is(closeTo(1.25, 0.001)));
  }

  @Test
  public void computes_hot_chocolate_with_cinnamon_price() {
    Beverage hotChocolateWithCinnamon = BeverageMachine.hotChocolate().withCinnamon().make();
    Assert.assertThat(hotChocolateWithCinnamon.price(), is(closeTo(1.50,0.001)));
  }
}

Notice the fluent interface that we decided for the builder. Although a fluent interface is not a requirement to write a builder, we think it reads nice.

As we said before, using a builder does not suffer from the combinatorial explosion of methods that the factory pattern did. The builder design pattern is more flexible than the factory pattern which makes it more suitable for composing components and decorators.

Still, our success is only partial because the builder can create any combination of beverages and supplements. A drawback of using a builder instead of a factory is usually that clients require to have more domain knowledge. In this case, the current solution forces the client code to hold a bit of domain knowledge: it knows which combinations of beverages and supplements are available on the menu.

We’ll fix this last problem in the next section.

A hybrid solution combining factory and builder patterns.

Let’s try to limit the possible combinations of beverages and supplements to the options on the menu by combining the creation methods of the factory pattern and the builder design pattern.

To do so, we added to BeverageMachine the creation methods, coffee, tea and hotChocolate, that create different builders for each type of beverage: CoffeeBuilder, TeaBuilderand HotChocolateBuilder, respectively. Each of the builders has only the public methods to select the supplements which are possible on the menu for a given type of beverage.

package coffee_shop;

import coffee_shop.beverages.Coffee;
import coffee_shop.beverages.HotChocolate;
import coffee_shop.beverages.Tea;
import coffee_shop.supplements.WithCinnamon;
import coffee_shop.supplements.WithCream;
import coffee_shop.supplements.WithMilk;

public class BeverageMachine {
  public static CoffeeBuilder coffee() {
    return new CoffeeBuilder();
  }

  public static TeaBuilder tea() {
    return new TeaBuilder();
  }

  public static HotChocolateBuilder hotChocolate() {
    return new HotChocolateBuilder();
  }

  public static class CoffeeBuilder {
    private Beverage beverage;

    private CoffeeBuilder() {
      this.beverage = new Coffee();
    }

    public Beverage make() {
      return beverage;
    }

    public CoffeeBuilder withMilk() {
      beverage = new WithMilk(beverage);
      return this;
    }

    public CoffeeBuilder withCinnamon() {
      beverage = new WithCinnamon(beverage);
      return this;
    }

    public CoffeeBuilder withCream() {
      beverage = new WithCream(beverage);
      return this;
    }
  }

  public static class TeaBuilder {
    private Beverage beverage;

    private TeaBuilder() {
      this.beverage = new Tea();
    }

    public Beverage make() {
      return beverage;
    }

    public TeaBuilder withMilk() {
      beverage = new WithMilk(beverage);
      return this;
    }

    public TeaBuilder withCinnamon() {
      beverage = new WithCinnamon(beverage);
      return this;
    }
  }

  public static class HotChocolateBuilder {
    private Beverage beverage;

    private HotChocolateBuilder() {
      this.beverage = new HotChocolate();
    }

    public Beverage make() {
      return beverage;
    }

    public HotChocolateBuilder withCinnamon() {
      beverage = new WithCinnamon(beverage);
      return this;
    }

    public HotChocolateBuilder withCream() {
      beverage = new WithCream(beverage);
      return this;
    }
  }
}

Notice that we chose to write the builders as inner classes of the BeverageMachine class. They could have been independent classes, but we prefer inner classes because the builders are only used by BeverageMachine and this way they don't appear anywhere else.

This is the first design that solves the problem of limiting the possible combinations of beverages and supplements to only the options on the menu. It still encapsulates the creational logic and still reads well. In fact the tests haven't changed at all because BeverageMachine’s public interface is exactly the same.

However, the new builders present duplication: the code related to supplements that can be used with different beverages and the code in the make method.

What is different for the clients that call the coffee method and the clients that call the tea or hotChocolate methods are the public methods they can use on each builder, that is, their interfaces. When we had only one builder, we had an interface with methods that were not interesting for some of its clients.

By having three builders we segregated the interfaces so that no client was forced to depend on methods it does not use^[9]. However we didn’t need to introduce classes to segregate the interfaces, we could have just used, well, interfaces. As we’ll see in the next section using interfaces would have avoided the duplication in the implementation of the builders.

Segregating interfaces better by using interfaces.

As we said, instead of directly using three different builder classes, it’s better to use three interfaces, one for each kind of builder. That would also comply with the Interface Segregation Principle, but, using the interfaces helps us avoid having duplicated code in the implementation of the builders, because we can write only one builder class, Beverage Machine, that implements the three interfaces.

package coffee_shop;

import coffee_shop.menu.beverages.Coffee;
import coffee_shop.menu.beverages.HotChocolate;
import coffee_shop.menu.beverages.Tea;
import coffee_shop.menu.beverages_builders.CoffeeBuilder;
import coffee_shop.menu.beverages_builders.HotChocolateBuilder;
import coffee_shop.menu.beverages_builders.TeaBuilder;
import coffee_shop.menu.supplements.WithCinnamon;
import coffee_shop.menu.supplements.WithCream;
import coffee_shop.menu.supplements.WithMilk;

public class BeverageMachine implements TeaBuilder, HotChocolateBuilder, CoffeeBuilder {
    private Beverage beverage;

    public static CoffeeBuilder coffee() {
        return new BeverageMachine(new Coffee());
    }

    public static TeaBuilder tea() {
        return new BeverageMachine(new Tea());
    }

    public static HotChocolateBuilder hotChocolate() {
        return new BeverageMachine(new HotChocolate());
    }

    private BeverageMachine(Beverage beverage) {
        this.beverage = beverage;
    }

    public Beverage make() {
        return beverage;
    }

    public BeverageMachine withMilk() {
        beverage = new WithMilk(beverage);
        return this;
    }

    public BeverageMachine withCinnamon() {
        beverage = new WithCinnamon(beverage);
        return this;
    }

    public BeverageMachine withCream() {
        beverage = new WithCream(beverage);
        return this;
    }
}

Notice how, in the creation methods, we feed the base beverage into BeverageMachine through its constructor, and how each of those creation methods return the appropriate interface. Notice also that BeverageMachine’s public interface remains the same, so this refactor won’t change the tests at all. You can check the resulting builder interfaces in Gist: TeaBuilder, HotChocolateBuilder and CoffeeBuilder.

Conclusions.

In this last post of the series dedicated to the Beverages Prices Refactoring kata, we’ve explored different ways to avoid creation sprawl, reduce coupling with client code and reduce implicit creational domain knowledge in client code. In doing so, we have learned about and applied several creational patterns (factory pattern, and builder design pattern), and some related refactorings. We have also used some design principles (such as coupling, open-closed principle or interface segregation principle), and code smells (such as combinatorial explosion or creation sprawl) to judge different solutions and guide our refactorings.

Acknowledgements.

I’d like to thank the WTM study group, and especially Inma Navas for solving this kata with me. Thanks also to my Codesai colleagues and to Inma Navas for reading the initial drafts and giving me feedback and to Amelia Hallsworth for the picture.

Notes.

[1] Knowledge here means coupling or connascence.

[2] Creation Sprawl is a code smell that happens when the knowledge for creating an object is spread out across numerous classes, so that creational responsibilities are placed in classes that should now be playing any role in object creation. This code smell was described by Joshua Kerievsky in his Refactoring to Patterns book.

[3] Don’t confuse the Factory Pattern with design patterns with similar names like Factory method pattern or Abstract factory pattern. These two design patterns are creational patterns described in the Design Patterns: Elements of Reusable Object-Oriented Software book.

A Factory Method is “a non-static method that returns a base class or an interface type and that is implemented in a hierarchy to enable polymorphic creation” whereas an Abstract Factory is “an interface for creating fqamiñlies of related or dependent objects without specifying their concrete classes”.

In the Factory Pattern a Factory is “any class that implements one or more Creation Methods” which are “static or non-static methods that create and return an object instance”. This definition is more general. Every Abstract Factory is a Factory (but not the other way around), and every Factory Method is a Creation Method (but not necessarily the reverse). Creation Method also includes what Martin Fowler called “factory method” in Refactoring (which is not the Factory Method design pattern) and Joshua Bloch called “static factory” (probably a less confusing name than Fowler’s one) in Effective Java.

[4] Presented in the fourth chapter of Refactoring to Patterns that is dedicated to Creational Patterns.
[5] If you remember the previous post, before introducing the decorator design pattern, we suffered from a combinatorial explosion of classes (adding a new supplement meant multiplying the number of classes by two). Now, the factory interface (its public methods) would suffer a combinatorial explosion of methods.

[6] In other words: that it’s [protected against that type of variation (https://www.martinfowler.com/ieeeSoftware/protectedVariation.pdf).

[7] This is common when working with creational patterns. All of them encapsulate knowledge about which concrete classes are used and hide how instances of these classes are created and put together, but some are more flexible than others. It’s usual to start using a Factory pattern and evolve toward the other creational patterns as we realize more flexibility is needed.

[8] We have devoted several posts to builders: Remove data structures noise from your tests with builders, Refactoring tests using builder functions in Clojure/ClojureScript, In a small piece of code, The curious case of the negative builder.

[9] Following the Interface Segregation Principle that states that “no client should be forced to depend on methods it does not use”.