This is the first article in a series I'm writing to give practical advice to newer developers. I've found there are a lot of unwritten conventions or pieces of advice that we expect junior developers to "pick up along the way". Often, these are so entrenched in engineering culture, that they've turned into cliches, like "google your problem before you ask for help," that aren't elaborated on in a meaningful way. What types of problems can be solved by googling? What search terms do I use? How do I know what information is useful to solve my problem? As a senior engineer who has mentored engineers with less experience, I am going to attempt to elaborate on and document these as I encounter them.
Pull requests are one area with quite a few unspoken conventions
There are a number of articles out there on the mechanics of making a pull request (hereafter referred to as PR). There are also a lot of articles about how to review a PR and how to respond to PR review feedback. I would like to talk about the in-between step. You've pushed up your branch, you've made a PR – is it ready for review? This is what I do when I've pushed up a PR, before I request a review from my team.
1. Check for merge conflicts
Handily, Github tells you if your branch cannot be merged due to merge conflicts. The first thing I do after creating a PR is to check for and address these conflicts.
2. Check for failing continuous integration (CI) steps
Depending on where you work, you may have CI (Github Actions, Travis, Circle, Jenkins, etc...) that runs against your PR. Commonly, this runs a linter and/or unit tests against your PR, and would block it from being merged if one of those fails. Before requesting a review from my coworkers, I address any failing steps and make sure all of the checks are green.
3. Read through the PR, as if I was reviewing it myself
I check for things that need to be removed:
- Is there any commented out code?
- Are there any stray logs or debugger statements?
- Are there any changes that are unrelated to my PR? These may be changes to code that should be part of a different branch, or accidental newlines added somewhere along the way. I like to remove even accidentally added whitespace because it leaves the commit history cleaner when others may be looking at the change history for a file.
I check for things that need to be changed:
- Do I have any typos or misspellings in my variables or comments?
- Would my variable names make sense to a new person coming into the codebase with less context than I have?
- Do I have any code that looks copy-pasted that I could make more DRY?
I check for things that need to be added:
- Does the business logic in my code make sense from looking at it, or do I need to add some comments?
- Did I add a piece of code without adding a corresponding test for it?
Some of the above are subjective, and knowing when you need to make changes will come with experience. You may still get comments or change requests around those things when others are doing a review for you. But some are easy to catch if you know to look for them, and fixing them beforehand is an important step towards taking ownership of your code, and a sign to your coworkers that you value their time and have done your due diligence.
Top comments (0)