The challenge

9 min read6 hours ago

–

Press enter or click to view image in full size

In this article I want to focus on such an aspect of the daily development process as pull requests and code review: how to make these artefacts more readable, smaller and how to make the code review process more efficient, faster and more practical.

Many of us work on user stories or backlog items in general, and even if a user story is a well-defined piece of functionality it often still takes a few days or even sometimes weeks to make it done.

As a project manager you might often be left in the dark about progress.

As a** peer engineer** you might be struggling to perform a proper code review of a big chunk of work published in the form of a pull request at the end of the development cycle.

Finally, as** the author of** such a big** pull request** you might become overwhelmed by the comments and remarks of your peers and sometimes you might need to perform deep refactoring of your pull requests based on these comments. Or it could be worse: due to a large size of your changes, code review may lack the needed profundity and miss crucial points that require some re-work. This all creates intransparency, waste of resources, low-quality pull request reviews and additional suboptimal iterations of development.

A solution? The key is to supply **small chunks **of work, preferably fitting in one man-day’s work, i.e. the pull requests one produces should be sensibly small and reflect the result of one workday. This is not a new approach. It’s been adopted as a best practice by Google, Amazon, Atlassian… Prominent voices in the Software-engineering world such as Martin Fowler, Kent Beck and many others speak about the same practice. All of them advocate smaller units of work, and this is an industry standard per se.

Slicing work

“In general it’s better to err on the side of writing CLs that are too small vs. CLs that are too large.”

Google Engineering Practices — Small CLs* *https://google.github.io/eng-practices/review/developer/small-cls.html

How to produce **small **portions of work?

This seems to be a trivial question, but in practice it is not so easy. Each produced chunk of work should meet the following criteria:

• The entire project should remain compilable
• All previously existing project functionality should still remain intact

In a nutshell, each such change** should not break the project**. It can add something to it, but what you push into the develop branch should remain potentially production ready.

Let’s say we are building a standard layered architecture application:

Here each small box inside a layer illustrates a sub-unit: a class, for example.

Could we slice this work simply by units (for example, classes)? Let’s call this slicing method “horizontal”:

Press enter or click to view image in full size

This approach would have a few downsides:

• Classes may not necessarily have comparably equal size (complexity)
• Partially implemented, e.g. with a missing class from the same/bottom layer, the project would not simply compile.

Another method that we would rather want to adopt would be “vertical” slicing:

In this approach each slice would include a partial implementation of the units and each subsequent chunk of work includes an elaborated (refactored/augmented) implementation of the previously created units plus some new blocks.

But what would drive these slices? What would define the contents of these slices?

Here is where **BDD **comes into play.

Behaviour Driven Development as a solution

“Flow. All else being equal, smaller batches of value more frequently are worth more than larger batches of value less frequently”

Kent Beck — Thinking About Code Review* *https://tidyfirst.substack.com/p/thinking-about-code-review

One of the most important artefacts that BDD helps produce is a **list of acceptance criteria **expressed in the form of concrete examples of the users’ behaviour. What does it have to do with our problem? To illustrate this, let’s take a concrete example. I asked ChatGPT to generate this example for me:

Feature: Shopping cart managementIn order to purchase products onlineAs a customerI want to add, remove, and update items in my shopping cartBackground:Given I am a registered customerAnd I have logged inAnd I have an empty shopping cartScenario 1: Add a single product to the cartWhen I add "Wireless Mouse" priced at 25 EUR to the cartThen the cart should contain 1 itemAnd the total price should be 25 EURScenario 2: Add multiple products to the cartWhen I add "Wireless Mouse" priced at 25 EUR to the cartAnd I add "Mechanical Keyboard" priced at 70 EUR to the cartThen the cart should contain 2 itemsAnd the total price should be 95 EURScenario 3: Update the quantity of an item in the cartGiven I already added "Wireless Mouse" priced at 25 EUR to the cartWhen I change the quantity of "Wireless Mouse" to 3Then the cart should contain 1 itemAnd the total price should be 75 EUR

(The rest of scenarios are omitted in order to be concise).

Looking at this list of scenarios suggests naturally that each scenario can be treated as a sub-task for a developer, it represents a sub-feature or a small piece of functionality. And this is how Acceptance Test Driven Development (ATDD) works. ATDD is a core development part of BDD methodology. ATDD is a generalisation of the Test Driven Development technique, but essentially it operates on all the same principles.

Mathematically speaking, TDD incrementally increases the complexity of the “automaton” your program represents. Constants are transformed into conditional statements; conditional statements are transformed into loops, etc. In practice it means that your very first pull request, implementing Scenario 1, will contain a very simple and straightforward implementation of the required logic.

Press enter or click to view image in full size

First comes an integration Spring Boot test:

@Testvoid scenario1_addSingleProductToCart() {    // Given    var addItem = AddItemRequest.builder()      .name("Wireless Mouse")      .price(Money.builder()      .amount(new BigDecimal("25.00"))      .currency("EUR")      .build())      .build();    // When    var addResp = rest.postForEntity(baseUrl() + "/api/carts/" + cartId + "/items", addItem, CartResponse.class);    assertThat(addResp.getStatusCode().is2xxSuccessful()).isTrue();    // Then    var cart = rest.getForObject(baseUrl() + "/api/carts/" + cartId, CartResponse.class);    assertThat(cart).isNotNull();    assertThat(cart.items).hasSize(1);    assertThat(cart.items.get(0).name).isEqualTo("Wireless Mouse");    assertThat(cart.totalPrice.currency).isEqualTo("EUR");    assertThat(cart.totalPrice.amount).isEqualByComparingTo("25.00");}

The simplest implementation of the controller will contain something like this:

@PostMapping("/{cartId}/items")public ResponseEntity<CartResponse> addItemToCart(@PathVariable UUID cartId, @RequestBody AddItemRequest request) {  return ResponseEntity.ok().build();}@GetMapping("/{cartId}")public ResponseEntity<CartResponse> getCart(@PathVariable UUID cartId) {  CartItem item = CartItem.builder()    .id(UUID.randomUUID().toString())    .name("Wireless Mouse")    .price(Money.builder()    .amount(new BigDecimal("25.00"))    .currency("EUR")    .build())    .quantity(1)    .build();    CartResponse cart = CartResponse.builder()    .id(cartId)    .items(Collections.singletonList(item))    .totalPrice(Money.builder()    .amount(new BigDecimal("25.00"))    .currency("EUR")    .build())    .build();   return ResponseEntity.ok(cart);}

As you can see, the addItemToCart method does almost nothing and **getCart **simply returns a pre-built object. Is it okay? It is okay for the first pull request as soon as our integration test passes.

We take the second scenario and create a new pull request, assuming that the previous one has been reviewed and merged into the develop branch.

Code Review

“Probably the most important practice to keep the code review process nimble is to keep changes small.”

Software Engineering at Google (book, Ch. 9: Code Review)*

• https://abseil.io/resources/swe-book

In my experience, I have often encountered engineers’ fear that such a dumb first implementation will be rejected by their peers. This is true if we consider a team not matured enough to apply TDD practices. However, the learning curve is not steep and a few workshops can educate and explain this practice. As soon as we have one-to-one correspondence between a single scenario from the acceptance criteria and a minimum code that was added to make it pass, such a pull request should be approved if this condition is met, among others industry best practices.

Thus the next pull request may take Scenario 2 for consideration. This will force the production code refactoring:

@Testvoid scenario2_addMultipleProductsToCart() {    // Given    var mouse = AddItemRequest.builder()      .name("Wireless Mouse")      .price(Money.builder()      .amount(new BigDecimal("25.00"))      .currency("EUR")      .build())      .build();    // When: I add "Wireless Mouse" priced at 25 EUR to the cart    var addMouseResp = rest.postForEntity(baseUrl() + "/api/carts/" + cartId + "/items", mouse, CartResponse.class);    assertThat(addMouseResp.getStatusCode().is2xxSuccessful()).isTrue();    // And I add "Mechanical Keyboard" priced at 70 EUR to the cart    var keyboard = AddItemRequest.builder()      .name("Mechanical Keyboard")      .price(Money.builder()      .amount(new BigDecimal("70.00"))      .currency("EUR")      .build())      .build();    var addKeyboardResp =    rest.postForEntity(baseUrl() + "/api/carts/" + cartId + "/items", keyboard, CartResponse.class);    assertThat(addKeyboardResp.getStatusCode().is2xxSuccessful()).isTrue();    // Then: the cart should contain 2 items and total price should be 95 EUR    CartResponse cart = rest.getForObject(baseUrl() + "/api/carts/" + cartId, CartResponse.class);    assertThat(cart).isNotNull();    assertThat(cart.items).hasSize(2);    assertThat(cart.items)    .extracting("name")    .containsExactlyInAnyOrder("Wireless Mouse", "Mechanical Keyboard");    assertThat(cart.totalPrice.currency).isEqualTo("EUR");    assertThat(cart.totalPrice.amount).isEqualByComparingTo("95.00");}

Now we can add service and repository layers:

@PostMapping("/{cartId}/items")public ResponseEntity<Void> addItemToCart(@PathVariable UUID cartId, @RequestBody AddItemRequest request) {  cartService.addItem(cartId, request);  return ResponseEntity.ok().build();}@GetMapping("/{cartId}")public ResponseEntity<CartResponse> getCart(@PathVariable UUID cartId) {  return ResponseEntity.ok(cartService.getCart(cartId));}

(I omit CartService and **CartRepository **source codes as this is for illustration purposes only).

“Making smaller pull requests is the #1 way to speed up your review time.”

Atlassian — The written/unwritten guide to pull requests* *https://www.atlassian.com/blog/git/written-unwritten-guide-pull-requests

Look at the new lines in the production code: these are the only changes in the controller class, hence it is easy to understand what sort of changes were performed and how the code evolves from the simplest implementation to something more production-ready.

The other benefit of small pull requests is quicker code review and** faster feedback** for the developers. My experience shows that many engineers often ignore pull request review requests and perform code review sometimes** many hours after the publishing**. This is understandable: they don’t want to be distracted; plunging oneself into the abyss of traditional huge changes is a task on its own and requires a significant context switch. The opposite situation comes with single-scenario based changes: here you have a concrete example of what we are automating and its small and concise implementation. **The automated test serves as documentation for the developed code. **This completely changes the culture of code review: someone from the team will always find time to give timely feedback. Even if the PR requires some rework, these comments will arrive fast, avoiding blocking the PR’s author and allowing him to adopt required modifications at an early stage of the development process.

Continuous Integration

“Everyone commits to the mainline every day… The key is to minimize the gap between integration, which is best done with small changes.”

**Martin Fowler — Continuous Integration **https://martinfowler.com/articles/continuousIntegration.html

“Working in smaller batches facilitates faster feedback, which is critical for continuous improvement. By reducing the size of work items completed, agile teams can swiftly gather and evaluate feedback.”

Michelle Wong — A Deep Dive into SAFe 6.0 Flow Metrics®: Maximizing Software Delivery Efficiency

https://www.planview.com/resources/articles/deep-dive-into-safe-6-flow-metrics/#5

Speaking of feedback, short PRs based on scenarios facilitate receiving fast reaction from the end users who have access to the DEV/TEST environment. Assuming that every merge to the develop branch in your CI/CD pipeline triggers a deployment on one of the pre-production environments, you get users’ feedback per scenario if needed! This is crucially important in Agile development cultures and should be appreciated by product owners as well.

Instead of conclusion

While googling for funny memes about pull requests size and code review, I accidentally stumbled across a blog dedicated to ECG Interpretation: https://ecg-interpretation.blogspot.com/2014/05/

It turns out this domain operates the same abbreviation “PR” which means, of course, a completely different thing. More specifically they speak about “PR interval” -a transmission delay — how long it takes for the electrical

signal to travel from the atria to the ventricles.

Picture borrowed from https://www.nottingham.ac.uk/nursing/practice/resources/cardiology/function/normal_duration.php

*“*The PR Interval: What is Long? What is Short?

For practical purposes — the PR interval in adults should normally not be more than 1 large box (0.20 second) in duration***”***

Bingo! This is exactly what we need. The normal work, cadence of an Agile team can be represented as a symphony of pulses, where this “P” wave corresponds to a pull request and… let me fantasise a bit:

• QRS — Quality Reassurance Stage, code review simply. So when you’ve done with another pull request and waiting for its approval — check your peers’ PR-s!
• QT — Query Task or Query Test (next scenario from the acceptance criteria list).

In essence, long PR (intervals) are not normal! Remember the TDD mantra:

Minute-by-Minute: micro-cycle: Red-Green-Refactor

(see this great Uncle Bob’s article: https://blog.cleancoder.com/uncle-bob/2014/12/17/TheCyclesOfTDD.html).

These cycles are a pulsation of an engineer’s daily work.

So let’s keep our ECG (Engineers Code Guidelines?) healthy and not let PRs be too long.