Code reviews

Feature branches

After assigning a task to myself and marking it "in progress", I left a comment on it estimating the amount of time I will spend on it:

/estimate 30m

When I am ready to start working on the feature, I make sure the project has been cloned locally, check out a new branch prefixed with the issue number (this one was called 4-code-reviews) and start implementing the feature or bug fix.

One ticket per branch

As a codebase grows, it is increasingly important to be able to refer changes back to the ticket in the scope of which they were implemented. As such, it is a good practice to create a new branch for every ticket you work on - merges and commit messages are easy to keep track of in git history and can prove invaluable when trying to track down the problem the clever hack you committed five months ago and now have to work around was supposed to solve. Having each feature implemented in a separate ticket also makes releasing specific changes trivially easy - if you have set up continuous integration/continuous delivery pipelines for your application, merging your feature branch to eg. main branch would usually trigger a build and deployment to one or multiple enviroments.

Commit messages

Rules for writing commit messages vary per team but they often follow this format:

When merged, this commit will {your message here}

For example, the commit message for this one will be something along the lines of add a section about commit messages. Whether or not your team will be strict about this, strive to document what you did - commit messages like fix, fix to prev fix and aargh would this please work already? It is 1AM, I am hungry and my patience is wearing thin provide little to no value for the next person (or you in three months - refer to Mr. Parker's obvious confusion in the image above!) looking at the code. Depending on integrations your team has set up, you might also want to refer to specific tickets - fix the first half of #4 - so that they show up in your project management software.

In case you want to include a longer explanation or a bulleted list of issues addressed in your commit message, break it into separate lines - the headline of your commit message should fit within 100 characters. Different code editors/IDE-s render blame in their own ways but short and straight to the point usually provides the most value at a glance.

Pull/merge requests and code reviews

Once I've finished writing this text and proofread it once or seventeen times, it is time to ask my colleagues to review it before merging it upstream. This is done via Merge Requests (Pull Requests in GitHub). The format of these also wildly varies between teams/employers but generally contains some freeform text about what was done and why, a link to the relevant ticket or related merge requests, some steps to verify that the bug you worked on was actually fixed or that the feature works, etc. Repository owners/admins can make reviews required before the branch can be merged at all - this is to make sure that code that hits your servers or customers' web browsers has been looked at and verified by at least two people.

The stringency of code reviews - once again - varies per team. If you have been assigned a code review, it is usually a good idea to check out the branch locally and verify the changes (this is made infinitely easier if the developer implementing the feature/fix has added reproduction steps, as mentioned above) on your own machine. It could be that their code only behaved as expected on their very specific local setup, their very specific version of Internet Explorer 10 or under strict alignment of celestial bodies. It could also be that it works out of the box but you won't really know before you (or your paying customers - which do you prefer?) try. Also try and read the code in the merge request - does something odd catch your eye?

• Do you think things could have been implemented in a more efficient manner?

• Does the code have potentially unhandled edge cases?

• Has the new code been covered by tests?

• Do you simply have questions about how some conclusions were reached

• Does the code diverge from your team's agreed patterns and practices?

• Does the code have unnecessary comments or commented-out blocks of code left in it?

• Does the formatting of the code break any agreed rules?

• Has your teammate decided to give object-oriented programming a go in your JavaScript project?

• You probably shouldn't let them get away with it this time.

Leave comments! Very rarely do I see merge requests where everything is absolutely perfect and exhaustively understandable on the first iteration. If I get a question about a piece of particularly jumbled or hard to parse (for humans) code, I leave a comment asking about it. If the commiter can't exhaustively explain it, they probably don't understand it well enough for the piece of code to survive the review.

Footnotes

• A very useful (and equally annoying) tool I have used in NodeJS/JavaScript projects is commitlint. This makes sure you write decent commit messages by simply not letting you commit unless your message meets certain criteria - header length, prefixes (eg. feat(search): implement caching or fix: remove potential memory leak in registration) etc. While it was a bit hard to come to terms with at first, it only took a few days to start seeing why my employer chose to implement it - whichever line of code I was looking at, I could look at the commit message from when it was added/modified and immediately know why it was written.

• When starting a new developer job, be it your first one or your fifth run as a principal engineer, make sure to take the time to understand your company's/team's commit practices. It can seem like something insignificant but teams are usually fairly confident in the processes they follow and the quicker you follow suit, the quicker you'll become a productive member of the team.