Curtis is working on a Code Review course.

It made me wanna write my own piece, a guide to pull requests. I'm no genius on this, but I think I can surely share from my experience what has worked well, and what I ideally enjoy, so yeah, this piece is quite subjective I must admit.

The ideal goal in software development is to be agile, ship fast with quality.

I'll be covering the process from a pull request being created all the way to merged I guess.

Author

Alright, let's start with authoring a pull request.

How I like to do it:

1. Before committing, I will run the tests, linter, etc.

2. When committing, I'll check my own diffs in VS Code.

3. After having submitted a pull request, I will again in Github/GitLab check my own diffs as well as check the ticket's requirements and make sure I implemented the stuff I'm supposed to.

4. I will then run related E2E Tests for the feature I was working on (running all of them would take a long time).

When it comes to committing, I try to make my commits small and follow Conventional Commits.

Now usually after this, I would share the pull request with co-workers in Slack.

Reviewer

Reviewing, let's go.

1. First thing I usually do is to check what it is about, the description, the commits, and then open the related Ticket for the pull request, to see the requirements, so I can later make sure they all have been implemented.

2. Review the code itself. When it comes to reviewing code, make sure to focus on the code and not the author:

Instead of for example saying:

Why did you implement this here?

...you may say:

I'm curious, why was this part implemented?

Don't forget, in the end, you are a team, you and your teammate have the same mission, be humble, open-minded, and also willing to share your knowledge as well as learn.

I enjoy asking questions when reviewing, and not making assumptions, I feel like that makes me come across as more humble, understanding that I don't know the intent of the author.

Examples:

I'm curious, why is optional chaining necessary here?

I'm wondering if this is necessary, considering we have a similar function in util.ts?

I have seen this being repeated several times in our codebase, perhaps we could have a look and see if it can be abstracted into a single component?

I also enjoy making suggestions when it fits, of how something may look like that I had in mind. If I'm for example proposing an abstraction, I would then post some code in the comment of how it could look like and perhaps the file name I had in mind.

1. If tests haven't been implemented, I ask for them to be implemented if it makes sense. If for example a new feature with some of its own edge cases has been implemented, then it does make sense, but if it is a minor refactoring or CSS change, then obviously that's different.

2. Check out the branch and check the code locally, if something is UI-related, make sure it works properly and looks nice matching the design.

Pair Reviews

To be honest, doing code reviews in an async flow, that has most of the times from my experience been quite slow, at times confusions rising, and I feel like pair reviews are perfect, and never have I regretted doing them.

Author

If you author a pull request, when sharing it, ping one of your teammates Slack/Discord and ask them to do a pair review with you, instead of waiting for someone to review your code.

If they can't do it right away, it's best to talk about when to do the pair review.

Example from your teammate:

I can't do it right now, let's review it together in 45 mins.

Reviewer

For the reviewer, quite the same, just ping the author and ask them to do a pair review with you, or communicate when it is possible for you to do it.

My Experience

Pair reviews have been amazing. I love easily you can talk through things, ask questions, and exchange ideas right away if something is supposed to have been implemented differently.

It still blows my mind how much smoother and faster the flow is compared to an async code review, where you'd be writing comments back and forth with your teammate.

I have also noticed as the reviewer, that the author better understands the Ticket and its requirements, leading to fewer initial questions being asked from me as a reviewer at times.

Doing an async code review flow, I've noticed, sometimes the pull request either gets forgotten or ignored, leading to it being reviewed quite late after the author reminds the team to review their pull request.

Conclusion

I love pair reviews, and for now, will always advocate for them, my experience doing pair reviews has been great.

Obviously depending on whom you are working with, ideally, you'd wanna work with teammates who are humble and open-minded, and willing to embrace change.