Effective code reviews with PR chunks

One of the problems I've come across working with multiple large teams is code reviews, more often than not you're working on a repo that you don't own, and the reviewer may or may not have context on the feature you're working on

I previously developed a full feature in 1 pull request, depending on the situation this PR can easily surpass 20 files, and being on the receiving end on these types of PRs, I can understand the frustration of reviewing them

You can easily lose track of the flow, as the files are sorted alphabetically, and you could jump from a test file to a config file then to the actual logic etc

The solution

PR chunks is what I like to call them, I don't know if it's the official term, but in essence, we part out our changes into different branches

Let's keep a running example, let's assume we're working on a web app, this App is MVC and uses the repository pattern for interacting with the database

Our task is to create an API that soft deletes posts, and send an email to the admin to inform him of the deletion

Splitting our solution

Upon initial inspection, our change can't be more than a few files right?

Wrong! you're forgetting tests, which will easily double the amount of files changed

Regardless of how our app is structured, we can logically split our feature into sections

• The database layer, where we will actually delete the post
• The 3rd party layer where we will call an API to do something (if needed)
• The business logic layer where we process the input before communicating with the database
• The entry point layer where we receive the request
• The routing layer where we define the endpoint

Using these sections, we can have PRs for each section to minimize the amount of changes per PR

ⓘ Note

You can have more or less layers, the above examples are not concrete, instead they serve as a thought process on how we're disecting the example task

But where to start? we start from the independent changes down to the dependent ones

So in our case we'd go

1. Database layer: Add deletion function to the post repository
2. 3rd party layer: Add a client to call the email service
3. Business logic layer: The service that calls the repository and the email service
4. The controller: where we call the service
5. The router: where we add the endpoint to delete the post and attaching the route to the controller method

Database changes

To start we create a branch, and give it a name corresponding to the task, and add a postfix to indicate that you're only doing database changes

git checkout -b add-post-deletion-database-changes

Then we add the database logic to the repository, here's some sample go code

func (p PostRepository) delete(id int) error {
    _, err := db.Exec("DELETE FROM posts WHERE id = ?", id)

    return err
}

Then we add a test for this method, making sure we test both when the id exists or doesn't exist

Also, there can be an interface defined for the repo, so we need to be adding this method's signature to the changes as well

In the end, we push our code and open up a PR

Until we get a review, we can asynchronously start working on the next step, which is

API client

We create a new branch from the previous branch

git checkout -b add-post-deletion-email-api-client

And add the integration with the email service, this will need a struct for the body, as an example

type SendEmailBody struct {
    Title string
    Body  string
    To    string
    Cc    []string
    Bcc   []string
}

And integrating with the service by calling the API and passing the body, then adding tests (don't forget mocking the API, we don't want to hit a live service in the suite!)

You can keep this locally until the first PR is merged, or open up a draft PR but don't request a review until the previous PR is merged

What happens when my first PR gets a change request?

Simple, address the comments and apply the changes, and then sync up the dependent branches

When the first PR gets merged, your next PR will only contain the changes that you did in the branch, which then can be ready for review

You're free to do the whole thing async like this or stop at specific parts to ensure you're not forgetting something, it's all up to you

Credits

Commit git icons created by Grand Iconic - Flaticon